RFR: JDK-8007607

[Chris Hegarty]

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
>