RFR: JDK-8007607
Valerie (Yu-Ching) Peng
valerie.peng at oracle.com
Fri Mar 15 02:06:27 UTC 2013
Looks good to me.
Thanks,
Valerie
On 03/14/13 06:04, John Zavgren wrote:
> Valerie: Thanks for catching this inconsistency. I just posted a
> modified webrev image:
> http://cr.openjdk.java.net/~jzavgren/8007607/webrev.08/ John
> ----- Original Message -----
> From: valerie.peng at oracle.com
> To: john.zavgren at oracle.com
> Cc: security-dev at openjdk.java.net
> Sent: Wednesday, March 13, 2013 9:07:45 PM GMT -05:00 US/Canada Eastern
> Subject: Re: RFR: JDK-8007607
>
>
> 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/20130314/3e725556/attachment.htm>
More information about the security-dev
mailing list