<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<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
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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jzavgren/8007607/webrev.04/">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/"><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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jzavgren/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 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 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 class="moz-txt-link-abbreviated" href="mailto:john.zavgren@oracle.com">john.zavgren@oracle.com</a>
603-821-0904
US-Burlington-MA</pre>
</body>
</html>