Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c

[Chris Hegarty]

On 22/10/2012 14:42, John Zavgren wrote:
> So, the most good we can do here is to make the reasons for errors correct... when the exceptions are thrown, be sure that the corresponding explanations makes sense and are not misleading?

This is common practice where a Java library class some native 
implementation. If your native code tries to access a field that is not 
declared, then it is simply an error. Clean up any native resources and 
return quickly.

The GetFieldID function will set a pending exception on the stack, this 
exception will be thrown during the transition back to Java-land. 
Anything else ( in this case ) would simply be wrong, and may hide, or 
make it more difficult to diagnose, a real bug.

-Chris.

>
> John
>
>
> ----- Original Message -----
> From: chris.hegarty at oracle.com
> To: john.zavgren at oracle.com
> Cc: Dmitry.Samersoff at oracle.com, security-dev at openjdk.java.net
> Sent: Monday, October 22, 2012 9:10:59 AM GMT -05:00 US/Canada Eastern
> Subject: Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c
>
>   >  When "file ID equals zero" we return before the "ThrowNew()" call is
> made. Should this call be moved to immediately before the "goto"
> statement? I.e.
>   >           if (fid == 0) {
>   >               jclass newExcCls =
>   >                   (*env)->FindClass(env,
> "java/lang/IllegalArgumentException");
>   >               if (newExcCls == 0) {
>   >                   /* Unable to find the new exception class, give up. */
>   >                   (*env)->ThrowNew(env, newExcCls, "invalid field:
> username");
>   >                   goto cleanUpAndReturn;
>   >               }
>   >           }
>
> We cannot do this since the we could pass a null reference to ThrowNew
> (through newExcCls).
>
> In fact a closer look shows that this code is very fragile. If
> GetFieldID returns null, then there will be a pending exception on the
> stack. It is an error/bad to call another JNI function if there is an
> exception on the stack. Also, isn't this the exact exception that should
> be thrown in this case? It is an error on the side of the JDK developer
> if any of these fields ID checks fail.
>
> We should simply do:
>       fid = (*env)->GetFieldID(env, cls, "uid", "J");
>       if (fid == 0)
>           goto cleanUpAndReturn;
>
>    .. and forget the IAE lookup, etc..
>
> -Chris.
>
>
> On 22/10/2012 12:11, John Zavgren wrote:
>> Dmitry:
>>
>> I see what you mean, these error messages are quite misleading. I will change all three of them.
>> I propose the following changes:
>> 1.) line 88, change: "invalid field: username" to "invalid field: user ID". (In Unix a user ID is a numerical value, not a "name" or character string.)
>> 2.) line 103, change: "invalid field: username" to "invalid field: user group ID"
>> 3.) line 118, change: "invalid field: username" to "invalid field: groups" (Or maybe "invalid group object"????)
>>
>> And for your second point, "aborting" (actually returning) after (*env)->ThrowNew(...), lines 88, 103, and 118... that makes sense too.
>>
>>
>> Consider the first case:
>>           if (fid == 0) {
>>               jclass newExcCls =
>>                   (*env)->FindClass(env, "java/lang/IllegalArgumentException");
>>               if (newExcCls == 0) {
>>                   /* Unable to find the new exception class, give up. */
>>                   goto cleanUpAndReturn;
>>               }
>>               (*env)->ThrowNew(env, newExcCls, "invalid field: username");
>>           }
>> When "file ID equals zero" we return before the "ThrowNew()" call is made. Should this call be moved to immediately before the "goto" statement? I.e.
>>           if (fid == 0) {
>>               jclass newExcCls =
>>                   (*env)->FindClass(env, "java/lang/IllegalArgumentException");
>>               if (newExcCls == 0) {
>>                   /* Unable to find the new exception class, give up. */
>>                   (*env)->ThrowNew(env, newExcCls, "invalid field: username");
>>                   goto cleanUpAndReturn;
>>               }
>>           }
>> If I don't move the "ThrowNew()" statement then the calling code will never see the exception.
>>
>> Ideas?
>> Thanks!
>> John Zavgren
>>
>> ----- Original Message -----
>> From: Dmitry.Samersoff at oracle.com
>> To: john.zavgren at oracle.com
>> Cc: security-dev at openjdk.java.net
>> Sent: Monday, October 22, 2012 4:53:56 AM GMT -05:00 US/Canada Eastern
>> Subject: Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c
>>
>> John,
>>
>> Sorry for being later. Again, it's not to your changes but as well as
>> you are touching this code.
>>
>> 88, 103, 118: Type-o - we are checking for uid,gid,groups field,
>>       but exception says "invalid field: username" in all cases.
>>
>>       It's better to fix it as well.
>>
>> ??: Does it make sense to abort after (*env)->ThrowNew(...) ?
>>
>> -Dmitry
>>
>>
>> On 2012-10-20 00:28, John Zavgren wrote:
>>> Greetings:
>>> The following webrev image contains a fix for a memory leak that occurs in the procedure: Java_com_sun_security_auth_module_UnixSystem_getUnixInfo (JNIEnv *env, jobject obj) in the file: jdk/src/solaris/native/com/sun/security/auth/module/Unix.c
>>>
>>> http://cr.openjdk.java.net/~khazra/john/8000204/webrev/
>>>
>>> The leaked memory is associated with the pointer named "groups": gid_t *groups = (gid_t *)calloc(numSuppGroups, sizeof(gid_t));id
>>>
>>> The procedure in question exits in many places and in every case it's necessary to deallocate this memory. The leak occurred because returns were being made without freeing it. I fixed the leak by modifying the code so that there is a common "exit point", that is reached from these same places via goto statements, that performs this common function, immediately before the "return" statement.
>>>
>>> Thanks!
>>> John Zavgren
>>> john.zavgren at oracle.com
>>>
>>
>>