<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
Looks good to me.<br>
Thanks,<br>
Valerie<br>
<br>
On 03/14/13 06:04, John Zavgren wrote:
<blockquote cite="mid:f02b1008-5b95-44e7-9d60-15fe82729383@default"
type="cite">Valerie:
Thanks for catching this inconsistency. I just posted a modified
webrev image:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jzavgren/8007607/webrev.08/">http://cr.openjdk.java.net/~jzavgren/8007607/webrev.08/</a>
John
<br>
----- Original Message -----<br>
From: <a class="moz-txt-link-abbreviated" href="mailto:valerie.peng@oracle.com">valerie.peng@oracle.com</a><br>
To: <a class="moz-txt-link-abbreviated" href="mailto:john.zavgren@oracle.com">john.zavgren@oracle.com</a><br>
Cc: <a class="moz-txt-link-abbreviated" href="mailto:security-dev@openjdk.java.net">security-dev@openjdk.java.net</a><br>
Sent: Wednesday, March 13, 2013 9:07:45 PM GMT -05:00 US/Canada
Eastern<br>
Subject: Re: RFR: JDK-8007607<br>
<br>
<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.7080402@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 moz-do-not-send="true"
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.6080208@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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true"
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 moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:john.zavgren@oracle.com"
target="_blank">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" target="_blank">john.zavgren@oracle.com</a>
<br>
603-821-0904 <br>
US-Burlington-MA <br>
<br>
</blockquote>
</blockquote>
<br>
<br>
<pre class="moz-signature">--
John Zavgren
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:john.zavgren@oracle.com" target="_blank">john.zavgren@oracle.com</a>
603-821-0904
US-Burlington-MA</pre>
</blockquote>
<br>
</div>
</blockquote>
<br>
</body>
</html>