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

[Chris Hegarty]

 > 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
>>
>
>