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

[John Zavgren]

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
> 


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