RFR: JDK-8007607

Valerie (Yu-Ching) Peng valerie.peng at oracle.com
Thu Mar 14 01:07:44 UTC 2013


Looks fine, just a very minor nit.
<GSSLibStub.c>
- line 544: although the return value isn't really used, maybe you 
should return jlong_zero instead of -1 for consistency sake.

Thanks,
Valerie
On 03/12/13 08:34, John Zavgren wrote:
> Greetings:
> I  posted a new webrev image in response to the most recent round of 
> comments:
> http://cr.openjdk.java.net/~jzavgren/8007607/webrev.07/ 
> <http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.07/>
>
> Thanks!
> John
>
> On 02/19/2013 12:16 PM, Chris Hegarty wrote:
>> Hi John,
>>
>> Functionally this looks fine. Most my comments are nit picking, and 
>> style.
>>
>> src/share/native/sun/security/jgss/wrapper/GSSLibStub.c
>>
>>   My fault, I think I suggested you return NULL, but since these
>>   methods return jlong they cannot (without generating warnings).
>>   It would be better to:
>>
>> < 332         return NULL;
>>    ---
>> > 332         return jlong_zero;
>>
>>    1167     return NULL;  [same comment, return jlong_zero]
>>
>>   The indentation looks a little too much in a few places, e.g.
>>     331   if ((*env)->ExceptionCheck(env)) {
>>     332   ______return NULL;
>>     333   }
>>
>>
>> src/share/native/sun/security/jgss/wrapper/NativeUtil.c
>>
>>   620     cOid = malloc(sizeof(struct gss_OID_desc_struct));
>>   621     if_(cOid == NULL) {   [add a space after if]
>>   622     ____throwOutOfMemoryError(env,NULL);  [I would suggest 2 
>> spaces]
>>   623         return GSS_C_NO_OID;
>>   624     }
>>   625     cOid->length = (*env)->GetArrayLength(env, jbytes) - 2;
>>   626     cOid->elements = malloc(cOid->length);
>>   627     if(cOid->elements == NULL) {        [ same as above ]
>>   628         throwOutOfMemoryError(env,NULL);
>>   629         free(cOid);
>>   630         return GSS_C_NO_OID;
>>   631     }
>>
>> src/share/native/sun/security/smartcardio/pcsc.c
>> src/solaris/native/sun/security/smartcardio/pcsc_md.c
>>
>>   It is kinda weird to have #ifdef WIN32 for these. It really seems
>>   that these functions should be moved up to the shared pcsc.c
>>   and externed from platform specific code, or an addition of
>>   pcsc.h that declares the definitions.
>>
>> src/solaris/native/com/sun/security/auth/module/Solaris.c
>>
>>   The comment is strange
>>   34 /*
>>   35  *  ** Throws a Java Exception by name
>>   36  *   **/
>>
>> src/solaris/native/com/sun/security/auth/module/Unix.c
>>
>>   Strange comment ( as above ). Also, why is there a need to for
>>   #ifndef  __solaris__ ??
>>
>> -Chris.
>>
>> On 02/18/2013 04:09 PM, John Zavgren wrote:
>>> Greetings:
>>> I posted a new webrev image.
>>> http://cr.openjdk.java.net/~jzavgren/8007607/webrev.04/
>>> <http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.04/>
>>>
>>> The code now throws an OOM exception when *alloc() fails, and the
>>> callers of procedures that call procedures that throw it, check for it.
>>>
>>> John
>>>
>>> On 02/12/2013 11:03 AM, Dmitry Samersoff wrote:
>>>> John,
>>>>
>>>> Changing everything for throw OOM is the right goal but it's a huge 
>>>> work
>>>> to do - it's meaningless to throw OOM if all callers doesn't check for
>>>> exception.
>>>>
>>>> I'm in doubt it could be done all at once and probably this problem
>>>> should go to the huge TODO pile.
>>>>
>>>> So I'm speaking about *one particular case*, where returned value 
>>>> could
>>>> lead to misinterpretation of security context and lead to security
>>>> vulnerability or subtle bug.
>>>>
>>>> We have to throw OOM there and change all callers as well for this 
>>>> case.
>>>>
>>>> -Dmitry
>>>>
>>>> On 2013-02-12 19:51, John Zavgren wrote:
>>>>> On 02/08/2013 01:34 PM, Dmitry Samersoff wrote:
>>>>>> John,
>>>>>>
>>>>>>> Ideas?
>>>>>> It's a JNI so just throw OOM.
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>>
>>>>>> On 2013-02-08 21:38, John Zavgren wrote:
>>>>>>> Although I agree that the name: "GSS_C_NO_CHANNEL_BINDINGS" is
>>>>>>> misleading,
>>>>>>> I can't identify anything else that seems more appropriate.
>>>>>>>
>>>>>>> The header file:
>>>>>>> /jdk8-tl/jdk/src/share/native/sun/security/jgss/wrapper/gssapi.h 
>>>>>>> defines
>>>>>>> GSS_C_NO_CHANNEL_BINDINGS as follows:
>>>>>>> #define GSS_C_NO_CHANNEL_BINDINGS ((gss_channel_bindings_t) 0)
>>>>>>>
>>>>>>> The symbol matches the prototype of the function:
>>>>>>>
>>>>>>>       */*
>>>>>>>        * Utility routine which creates a gss_channel_bindings_t 
>>>>>>> structure
>>>>>>>        * using the specified org.ietf.jgss.ChannelBinding object.
>>>>>>>        */
>>>>>>>       gss_channel_bindings_t getGSSCB(JNIEnv *env, jobject jcb) {
>>>>>>>         gss_channel_bindings_t cb;
>>>>>>>         jobject jinetAddr;
>>>>>>>         jbyteArray value;
>>>>>>>
>>>>>>>         if (jcb == NULL) {
>>>>>>>           return GSS_C_NO_CHANNEL_BINDINGS;
>>>>>>>         }
>>>>>>>           cb = malloc(sizeof(struct gss_channel_bindings_struct));
>>>>>>>
>>>>>>>           if(cb == NULL)
>>>>>>>               return  GSS_C_NO_CHANNEL_BINDINGS;*
>>>>>>>
>>>>>>> There doesn't appear to be anything in our set of options that 
>>>>>>> is more
>>>>>>> suggestive of a memory allocation failure and the symbol:
>>>>>>> GSS_C_NO_CHANNEL_BINDINGS seems to be logically correct.
>>>>>>>
>>>>>>> Ideas?
>>>>>>>
>>>>>>> On 02/06/2013 04:57 AM, Dmitry Samersoff wrote:
>>>>>>>> John,
>>>>>>>>
>>>>>>>> Not sure GSS_C_NO_CHANNEL_BINDINGS; is correct return value for 
>>>>>>>> this
>>>>>>>> case.
>>>>>>>>
>>>>>>>> I'm second to Valerie - it's better to throw OOM
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2013-02-06 03:44, John Zavgren wrote:
>>>>>>>>> Greetings:
>>>>>>>>>
>>>>>>>>> I modified the native code to eliminate potential memory loss and
>>>>>>>>> crashes by checking the return values of malloc() and 
>>>>>>>>> realloc() calls.
>>>>>>>>>
>>>>>>>>> The webrev image of these changes is visible at:
>>>>>>>>> http://cr.openjdk.java.net/~jzavgren/8007607/webrev.01/
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>> John Zavgren
>>>>>>>>>
>>>>>>> -- 
>>>>>>> John Zavgren
>>>>>>> john.zavgren at oracle.com
>>>>>>> 603-821-0904
>>>>>>> US-Burlington-MA
>>>>>>>
>>>>> When I change the procedures in the following files:
>>>>>
>>>>> src/share/native/sun/security/jgss/wrapper/GSSLibStub.c
>>>>> src/share/native/sun/security/jgss/wrapper/NativeUtil.c
>>>>> src/share/native/sun/security/smartcardio/pcsc.c
>>>>> src/solaris/native/com/sun/security/auth/module/Solaris.c
>>>>> src/solaris/native/com/sun/security/auth/module/Unix.c
>>>>>
>>>>> to fix inappropriate usages of malloc, realloc, etc. (e.g., not 
>>>>> checking
>>>>> the return value and referring to a NULL pointer and crashing) 
>>>>> should I
>>>>> modify every instance so that an OOM (Out Of Memory) exception is 
>>>>> thrown
>>>>> (before returning) whenever memory allocation fails?
>>>>>
>>>>> The exceptions would be thrown by a line of code that looks like:
>>>>>
>>>>> throwOutOfMemoryError(JNIEnv *env, const char *msg);
>>>>>
>>>>> where  throwOutOfMemoryError(...) wraps something like this:
>>>>>
>>>>>              jclass cls = (*env)->FindClass(env, name);
>>>>>
>>>>>                  if (cls != 0) /* Otherwise an exception has 
>>>>> already been
>>>>> thrown */
>>>>>                                  (*env)->ThrowNew(env, cls, msg);
>>>>>
>>>>> If this is the right idea, what messages should I pass when an OOM
>>>>> exception is thrown?
>>>>>
>>>>> Thanks!
>>>>> John
>>>>>
>>>>
>>>
>>>
>>> -- 
>>> John Zavgren
>>> john.zavgren at oracle.com
>>> 603-821-0904
>>> US-Burlington-MA
>>>
>
>
> -- 
> John Zavgren
> john.zavgren at oracle.com
> 603-821-0904
> US-Burlington-MA

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20130313/3fbc620a/attachment.htm>


More information about the security-dev mailing list