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

[John Zavgren]

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



-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...