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.p...@oracle.com
To: john.zavg...@oracle.com
Cc: security-dev@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.zavg...@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.zavg...@oracle.com
603-821-0904
US-Burlington-MA
--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA
