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