RFR: JDK-8007607
John Zavgren
john.zavgren at oracle.com
Tue Mar 12 15:34:00 UTC 2013
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/20130312/1b69b900/attachment.htm>
More information about the security-dev
mailing list