Re: 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.samers...@oracle.com
To: chris.hega...@oracle.com
Cc: john.zavg...@oracle.com, security-dev@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