<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<src/share/native/sun/security/jgss/wrapper/GSSLibStub.c><br>
- The implementation of throwByName(...) and throwOutOfMemory(...)
should be moved to NativeUtil.c as that's where most if not all the
utility functions are held.<br>
- I think it's better to move the block of code from line 330 to 333
to before the initGSSBuffer(...) call on line 329. Note that
initGSSBuffer(...) calls should be paired with resetGSSBuffer(...)
calls, so, easier/cleaner if you adjust the order here.<br>
- Same comment applies to line 927 and the code block of
line928-931. Swapping their order would save you the call of
resetGSSBuffer(...) calls.<br>
- why do you make change to line 1167?<br>
<br>
<src/share/native/sun/security/jgss/wrapper/NativeUtil.c><br>
- The extern declaration on line 31 should be moved to NativeUtil.h
and not here.<br>
- This is nothing to do with your change, but I noticed that there
should be a return statement following line618. No point to continue
to the rest of code, i.e. malloc and etc., if an exception has
already occurred.<br>
<br>
<src/solaris/native/com/sun/security/auth/module/Solaris.c><br>
- since there are only one calloc call, I don't see much value for
defining a local method. I think you can just follow the same
pattern of line 78-85. Less lines of code this way.<br>
<br>
<src/solaris/native/com/sun/security/auth/module/Unix.c><br>
- see above, same comment as Solaris.c.<br>
<br>
<src/share/native/sun/security/smartcardio/pcsc.c><br>
- line141, space between the "if" and "("?<br>
- line 195-196, I think the check here is somewhat redundant. If an
exception occurred in the pcsc_multi2jstring(...), then it should
free "mszReaders" before returning. The next two lines doing exactly
that, so I don't think adding a check here makes any difference.<br>
- line 311, it's cleaner to do the malloc call and the null-check
before calling (*env)->GetIntArrayElements(...) (you have on line
309) since it is paired with a
(*env)->ReleaseIntArrayElements(...) call.<br>
<b><br>
</b><src/solaris/native/sun/security/smartcardio/pcsc_md.c><br>
- There is no malloc calls in this file at all. So, I think you
should just move the <span class="new">throwOutOfMemoryError</span>
method to the "pcsc.c" file. Then you can get rid of the code block
from line 42 to 57. <br>
<br>
Thanks,<br>
Valerie<br>
<br>
On 02/18/13 08:09, John Zavgren wrote:
<blockquote cite="mid:51225229.6080807@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.<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.04/">http://cr.openjdk.java.net/~jzavgren/8007607/webrev.04/</a><br>
<br>
The code now throws an OOM exception when *alloc() fails, and
the 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>
</div>
<blockquote cite="mid:511A67C9.70503@oracle.com" type="cite">
<pre wrap="">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:
</pre>
<blockquote type="cite">
<pre wrap="">On 02/08/2013 01:34 PM, Dmitry Samersoff wrote:
</pre>
<blockquote type="cite">
<pre wrap="">John,
</pre>
<blockquote type="cite">
<pre wrap="">Ideas?
</pre>
</blockquote>
<pre wrap="">It's a JNI so just throw OOM.
-Dmitry
On 2013-02-08 21:38, John Zavgren wrote:
</pre>
<blockquote type="cite">
<pre wrap="">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:
</pre>
<blockquote type="cite">
<pre wrap="">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:
</pre>
<blockquote type="cite">
<pre wrap="">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:
<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>
Thanks!
John Zavgren
</pre>
</blockquote>
</blockquote>
<pre wrap="">--
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>
</blockquote>
<pre wrap="">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
</pre>
</blockquote>
<pre wrap="">
</pre>
</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>