<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
Looks fine, just a very minor nit.<br>
<GSSLibStub.c><br>
- line 544: although the return value isn't really used, maybe you
should return <span class="changed">jlong_zero instead of -1 for
consistency sake.</span><br>
<br>
Thanks,<br>
Valerie<br>
On 03/12/13 08:34, John Zavgren wrote:
<blockquote cite="mid:513F4AE8.7080402@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Greetings:<br>
I posted a new webrev image in response to the most recent
round of comments:<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.07/">http://cr.openjdk.java.net/~jzavgren/8007607/webrev.07/</a><br>
<br>
Thanks!<br>
John<br>
<br>
On 02/19/2013 12:16 PM, Chris Hegarty wrote:<br>
</div>
<blockquote cite="mid:5123B35C.6080208@oracle.com" type="cite">Hi
John, <br>
<br>
Functionally this looks fine. Most my comments are nit picking,
and style. <br>
<br>
src/share/native/sun/security/jgss/wrapper/GSSLibStub.c <br>
<br>
My fault, I think I suggested you return NULL, but since these
<br>
methods return jlong they cannot (without generating
warnings). <br>
It would be better to: <br>
<br>
< 332 return NULL; <br>
--- <br>
> 332 return jlong_zero; <br>
<br>
1167 return NULL; [same comment, return jlong_zero] <br>
<br>
The indentation looks a little too much in a few places, e.g.
<br>
331 if ((*env)->ExceptionCheck(env)) { <br>
332 ______return NULL; <br>
333 } <br>
<br>
<br>
src/share/native/sun/security/jgss/wrapper/NativeUtil.c <br>
<br>
620 cOid = malloc(sizeof(struct gss_OID_desc_struct)); <br>
621 if_(cOid == NULL) { [add a space after if] <br>
622 ____throwOutOfMemoryError(env,NULL); [I would suggest
2 spaces] <br>
623 return GSS_C_NO_OID; <br>
624 } <br>
625 cOid->length = (*env)->GetArrayLength(env,
jbytes) - 2; <br>
626 cOid->elements = malloc(cOid->length); <br>
627 if(cOid->elements == NULL) { [ same as above
] <br>
628 throwOutOfMemoryError(env,NULL); <br>
629 free(cOid); <br>
630 return GSS_C_NO_OID; <br>
631 } <br>
<br>
src/share/native/sun/security/smartcardio/pcsc.c <br>
src/solaris/native/sun/security/smartcardio/pcsc_md.c <br>
<br>
It is kinda weird to have #ifdef WIN32 for these. It really
seems <br>
that these functions should be moved up to the shared pcsc.c <br>
and externed from platform specific code, or an addition of <br>
pcsc.h that declares the definitions. <br>
<br>
src/solaris/native/com/sun/security/auth/module/Solaris.c <br>
<br>
The comment is strange <br>
34 /* <br>
35 * ** Throws a Java Exception by name <br>
36 * **/ <br>
<br>
src/solaris/native/com/sun/security/auth/module/Unix.c <br>
<br>
Strange comment ( as above ). Also, why is there a need to for
<br>
#ifndef __solaris__ ?? <br>
<br>
-Chris. <br>
<br>
On 02/18/2013 04:09 PM, John Zavgren wrote: <br>
<blockquote type="cite">Greetings: <br>
I posted a new webrev image. <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.04/">http://cr.openjdk.java.net/~jzavgren/8007607/webrev.04/</a>
<br>
<a moz-do-not-send="true" class="moz-txt-link-rfc2396E"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.04/"><http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.04/></a>
<br>
<br>
The code now throws an OOM exception when *alloc() fails, and
the <br>
callers of procedures that call procedures that throw it,
check for it. <br>
<br>
John <br>
<br>
On 02/12/2013 11:03 AM, Dmitry Samersoff wrote: <br>
<blockquote type="cite">John, <br>
<br>
Changing everything for throw OOM is the right goal but it's
a huge work <br>
to do - it's meaningless to throw OOM if all callers doesn't
check for <br>
exception. <br>
<br>
I'm in doubt it could be done all at once and probably this
problem <br>
should go to the huge TODO pile. <br>
<br>
So I'm speaking about *one particular case*, where returned
value could <br>
lead to misinterpretation of security context and lead to
security <br>
vulnerability or subtle bug. <br>
<br>
We have to throw OOM there and change all callers as well
for this case. <br>
<br>
-Dmitry <br>
<br>
On 2013-02-12 19:51, John Zavgren wrote: <br>
<blockquote type="cite">On 02/08/2013 01:34 PM, Dmitry
Samersoff wrote: <br>
<blockquote type="cite">John, <br>
<br>
<blockquote type="cite">Ideas? <br>
</blockquote>
It's a JNI so just throw OOM. <br>
<br>
-Dmitry <br>
<br>
<br>
On 2013-02-08 21:38, John Zavgren wrote: <br>
<blockquote type="cite">Although I agree that the name:
"GSS_C_NO_CHANNEL_BINDINGS" is <br>
misleading, <br>
I can't identify anything else that seems more
appropriate. <br>
<br>
The header file: <br>
/jdk8-tl/jdk/src/share/native/sun/security/jgss/wrapper/gssapi.h
defines <br>
GSS_C_NO_CHANNEL_BINDINGS as follows: <br>
#define GSS_C_NO_CHANNEL_BINDINGS
((gss_channel_bindings_t) 0) <br>
<br>
The symbol matches the prototype of the function: <br>
<br>
*/* <br>
* Utility routine which creates a
gss_channel_bindings_t structure <br>
* using the specified
org.ietf.jgss.ChannelBinding object. <br>
*/ <br>
gss_channel_bindings_t getGSSCB(JNIEnv *env,
jobject jcb) { <br>
gss_channel_bindings_t cb; <br>
jobject jinetAddr; <br>
jbyteArray value; <br>
<br>
if (jcb == NULL) { <br>
return GSS_C_NO_CHANNEL_BINDINGS; <br>
} <br>
cb = malloc(sizeof(struct
gss_channel_bindings_struct)); <br>
<br>
if(cb == NULL) <br>
return GSS_C_NO_CHANNEL_BINDINGS;* <br>
<br>
There doesn't appear to be anything in our set of
options that is more <br>
suggestive of a memory allocation failure and the
symbol: <br>
GSS_C_NO_CHANNEL_BINDINGS seems to be logically
correct. <br>
<br>
Ideas? <br>
<br>
On 02/06/2013 04:57 AM, Dmitry Samersoff wrote: <br>
<blockquote type="cite">John, <br>
<br>
Not sure GSS_C_NO_CHANNEL_BINDINGS; is correct
return value for this <br>
case. <br>
<br>
I'm second to Valerie - it's better to throw OOM <br>
<br>
-Dmitry <br>
<br>
<br>
On 2013-02-06 03:44, John Zavgren wrote: <br>
<blockquote type="cite">Greetings: <br>
<br>
I modified the native code to eliminate potential
memory loss and <br>
crashes by checking the return values of malloc()
and realloc() calls. <br>
<br>
The webrev image of these changes is visible at: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.01/">http://cr.openjdk.java.net/~jzavgren/8007607/webrev.01/</a>
<br>
<br>
Thanks! <br>
John Zavgren <br>
<br>
</blockquote>
</blockquote>
-- <br>
John Zavgren <br>
<a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:john.zavgren@oracle.com">john.zavgren@oracle.com</a>
<br>
603-821-0904 <br>
US-Burlington-MA <br>
<br>
</blockquote>
</blockquote>
When I change the procedures in the following files: <br>
<br>
src/share/native/sun/security/jgss/wrapper/GSSLibStub.c <br>
src/share/native/sun/security/jgss/wrapper/NativeUtil.c <br>
src/share/native/sun/security/smartcardio/pcsc.c <br>
src/solaris/native/com/sun/security/auth/module/Solaris.c
<br>
src/solaris/native/com/sun/security/auth/module/Unix.c <br>
<br>
to fix inappropriate usages of malloc, realloc, etc.
(e.g., not checking <br>
the return value and referring to a NULL pointer and
crashing) should I <br>
modify every instance so that an OOM (Out Of Memory)
exception is thrown <br>
(before returning) whenever memory allocation fails? <br>
<br>
The exceptions would be thrown by a line of code that
looks like: <br>
<br>
throwOutOfMemoryError(JNIEnv *env, const char *msg); <br>
<br>
where throwOutOfMemoryError(...) wraps something like
this: <br>
<br>
jclass cls = (*env)->FindClass(env, name);
<br>
<br>
if (cls != 0) /* Otherwise an exception
has already been <br>
thrown */ <br>
(*env)->ThrowNew(env,
cls, msg); <br>
<br>
If this is the right idea, what messages should I pass
when an OOM <br>
exception is thrown? <br>
<br>
Thanks! <br>
John <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
-- <br>
John Zavgren <br>
<a moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:john.zavgren@oracle.com">john.zavgren@oracle.com</a>
<br>
603-821-0904 <br>
US-Burlington-MA <br>
<br>
</blockquote>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
John Zavgren
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:john.zavgren@oracle.com">john.zavgren@oracle.com</a>
603-821-0904
US-Burlington-MA</pre>
</blockquote>
<br>
</body>
</html>