Valerie:
Thanks for catching this inconsistency. I just posted a modified webrev image:
http://cr.openjdk.java.net/~jzavgren/8007607/webrev.08/
John
<br>----- Original Message -----<br>From: valerie.p...@oracle.com<br>To:
john.zavg...@oracle.com<br>Cc: security-dev@openjdk.java.net<br>Sent:
Wednesday, March 13, 2013 9:07:45 PM GMT -05:00 US/Canada Eastern<br>Subject:
Re: RFR: JDK-8007607<br><br><html>
<head>
</head>
<div>
<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.7080...@oracle.com">
<div class="moz-cite-prefix">Greetings:<br>
I posted a new webrev image in response to the most recent
round of comments:<br>
<a href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.07/";
target="_blank">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.6080...@oracle.com">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>Greetings: <br>
I posted a new webrev image. <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.04/";
target="_blank">http://cr.openjdk.java.net/~jzavgren/8007607/webrev.04/</a>
<br>
<a class="moz-txt-link-rfc2396E"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.04/";
target="_blank"><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>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>On 02/08/2013 01:34 PM, Dmitry
Samersoff wrote: <br>
<blockquote>John, <br>
<br>
<blockquote>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>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>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>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 class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejzavgren/8007607/webrev.01/";
target="_blank">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 class="moz-txt-link-abbreviated"
href="mailto:john.zavg...@oracle.com";
target="_blank">john.zavg...@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 class="moz-txt-link-abbreviated"
href="mailto:john.zavg...@oracle.com";
target="_blank">john.zavg...@oracle.com</a>
<br>
603-821-0904 <br>
US-Burlington-MA <br>
<br>
</blockquote>
</blockquote>
<br>
<br>
<pre class="moz-signature">--
John Zavgren
<a class="moz-txt-link-abbreviated" href="mailto:john.zavg...@oracle.com";
target="_blank">john.zavg...@oracle.com</a>
603-821-0904
US-Burlington-MA</pre>
</blockquote>
<br>
</div>
</html>
