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

[Chris Hegarty]

John,

The existing/old code is actually wrong and would crash if the uid field 
did not exist. What Dmitry said in his last mail is the best approach.

-Chris

On 22/10/2012 15:35, John Zavgren wrote:
> Dmitry:
>
> You recommend that this procedure check the return value of the GetField() call and if there is an error (the return value is zero), clean up and return immediately and not try to create an instance of the exception class?
>
> The code that we have now will accept every possible return value of GetField() and set this value by calling: SetLongField() unless it can't create an exception object before it's able to actually call GetField().
>
> This is what I was proposing:
>          /*
>           * set uid
>           */
>          fid = (*env)->GetFieldID(env, cls, "uid", "J");
>          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: user ID");
>          }
>          (*env)->SetLongField(env, obj, fid, pwd->pw_uid);
> And, this is what I think you are proposing:
>          /*
>           * set uid
>           */
>          fid = (*env)->GetFieldID(env, cls, "uid", "J");
>          if (fid == 0) {
>                 goto cleanUpAndReturn;
>          }
>          (*env)->SetLongField(env, obj, fid, pwd->pw_uid);
>
> Thanks,
> John
> ----- Original Message -----
> From: Dmitry.Samersoff at oracle.com
> To: chris.hegarty at oracle.com
> Cc: john.zavgren at oracle.com, security-dev at openjdk.java.net
> Sent: Monday, October 22, 2012 10:18:56 AM GMT -05:00 US/Canada Eastern
> Subject: Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c
>
> Chris,
>
> On 2012-10-22 17:11, Chris Hegarty wrote:
>> We should simply do:
>>      fid = (*env)->GetFieldID(env, cls, "uid", "J");
>>      if (fid == 0)
>>          goto cleanUpAndReturn;
>>
>>   .. and forget the IAE lookup, etc..
>
>
> I'm second for simple code above if now spec requires IAE here.
>
> Also it's better to retrieve all fields first, before setting any values
> as it's good practice to don't modify client data on error.
>
> i.e.
>
> fid_username = (*env)->GetFieldID()
> if (fid_username == 0 )
>    goto cleanup_and_return;
>
> fid_uid = (*env)->GetFieldID()
> if (fid_uid == 0 )
>    goto cleanup_and_return;
>
> ....
>
> fid_gid = (*env)->GetFieldID()
> ....
>
> // Set all required fields here
>
> -Dmitry
>
>
>