RFR: JDK-8007607

Tue Feb 12 21:59:42 UTC 2013

John,

On 2013-02-12 21:11, John Zavgren wrote:
> If I were to rewrite the native code to throw exceptions, in the
> areas I indicated, and no one caught them, would that create any
> problems. I have the code in my sandbox right now for throwing them.
> If I can retain that in a graceful way, that seems like a prudent
> thing to do, if there are no negative consequences.

Java level catch OOM exception and Java program will do something
accordingly.

Unfortunately, native caller is not enforced to check for exception and
could do lots of nasty things if it doesn't check for exception.

Lets consider imaginary code below:

uint some_function(){
 void *p = malloc(...)
// do something
 return p->int_field;
}

void caller(){
    a = some_function()
    if (a > 0){
       // some code
    }

}

this code most probably crash on OOM and it's OK.
but after change to

uint some_function(){
 void *p = malloc(...)
 if (p == NULL){
     throw OOM
     return 1;
 }
// do something
 return p->int_field;
}

code become totally wrong and can do lots of nasty things ever if Java
level handle OOM late.

So all callers have to be carefully checked for interpretation of the
return value in case of OOM. If we can *semantically* recover on native
level we should do it.  If not - make sure that all callers do nothing
but pass exception to Java or leave program crashing.

e.g. enumerating network interfaces it probably OK to return incomplete
list in case of OOM and throw ООМ exception to let Java know that the
list is not complete, but enumerating domain names for particular IP,
we shouldn't return incomplete list because it used for some security
check and could lead to hardly reproducible application problems like
rejected e-mails.

-Dmitry

> 
> John
> 
> 
> ----- Original Message ----- From: dmitry.samersoff at oracle.com To:
> john.zavgren at oracle.com Cc: security-dev at openjdk.java.net Sent:
> Tuesday, February 12, 2013 11:04:28 AM GMT -05:00 US/Canada Eastern 
> Subject: Re: RFR: JDK-8007607
> 
> 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
>> 
> 
> 

-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer