<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>