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