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