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