278434 - JSS exceptions can cause crash in socketClose

Status: RESOLVED DUPLICATE of bug 282732

(JSS Graveyard :: Library, defect)

Description

[Saul Edwards]

The native getException call in JSS has appeared in several crashes over the
past year, i.e.:

--- called from signal handler with signal 10 (SIGBUS) ---
f994f2f8 JSS_SSL_getException (6567696f, ac8d0, fbed9f78, f9978964, 0, 1) + 30
f994f004 JSS_SSL_processExceptions (5705bc, 6567696f, f9978958, f9978964,
e007f884, 5b) + 1c4
f994e4dc Java_org_mozilla_jss_ssl_SocketBase_socketClose (5705bc, e007f94c, 0,
5b, 18, 15) + dc
f9f941cc ???????? (ee218840, e007f88c, 0, 2a, ed8df668, ed8df668) 
...

Looking at the code in the close method, this is probably happening because the
jSockPriv pointer is not set to null after the socket is closed with PR_Close. 
Therefore, getException will try to dereference the pointer to get the exception
member of the structure.  This will crash since PR_Close has already freed the
structure.

Comment 1

[Saul Edwards]

Attachment: Null jSockPriv, check NULL pointers in getException

Comment 2

[glen beasley]

Comment on attachment 174509 [details] [diff] [review]
Null jSockPriv, check NULL pointers in getException

In setException if priv is equal to NULL then you 
also need to protect  
priv->exception = excep. 

priv is a PRFilePrivate stucture, it get created 
py PR_NEW see:
http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/javasoc
k.c#978
Which means we need to free this structure before we NULL it in the close.

Comment 3

[glen beasley]

Attachment: protect set and get for exceptions, and modify socket.close

added null pointer checks in getException and setException
change socket.close to mark socket as closed and to free 
all resouces of the socket. The actual socket will get 
freed when releaseNativeResources gets called by the jvm
or a call to System.runFinalization(); or System.gc();

Comment 4

[glen beasley]

Attachment: added more comments and assert on jsockPriv

Comment 5

[Wan-Teh Chang]

Comment on attachment 174716 [details] [diff] [review]
added more comments and assert on jsockPriv

>+void JSSL_DestroySocketResources(JNIEnv *env, JSSL_SocketData *sd)

Should this new function be static?

>+    /*do not free sd->Object here since the socket still exists as closed*/

Should that be sd->socketObject?

>     if( sd->clientCertSlot != NULL ) {
>         PK11_FreeSlot(sd->clientCertSlot);
>+        sd->clientCertSelectionCallback = NULL;
>+    }

Should that be "sd->clientCertSlot = NULL;" ?

>+    if (sd->jsockPriv != NULL) {
>+        /*jsockPriv should always be null in common.c layer which operates */
>+        /*on socket created by PR_NewTCPSocket() in socketCreate or */
>+        /*socketAccept; */
>+        PR_ASSERT(sd->jsockPriv != NULL );
>+        sd->jsockPriv = NULL;
>+    }

That PR_ASSERT is useless.  It must be true
because it immediately follows an if statement
that guarantees it.

>+void
>+JSSL_DestroySocketData(JNIEnv *env, JSSL_SocketData *sd)
>+{
>+    PR_ASSERT(sd != NULL);
>+    /* if user did not call Socket.Close need to releaseResources */

I'm sorry.  I have trouble understanding this comment.
Perhaps add a comma (,) after "Socket.Close"?

>     if( ! sock->closed ) {
>-        PR_Close(sock->fd);
>-        sock->closed = PR_TRUE;
>-        /* this may have thrown an exception */
>+        /*user is closing socket. Destroy all related memory structures*/ 
>+        /* related to the socket because in high load the Garbage */
>+        /* Collection may not releaseNativeResources soon enough */
>+        /* releaseNativeResource will free the actual structure */
>+        JSSL_DestroySocketResources(env, sock);
>     }

What is "the actual structure"?

I see JSSL_DestroySocketData call PR_Free on
the JSSL_SocketData structure.	Does
releaseNativeResource(s?) call JSSL_DestroySocketData?

> void
> setException(JNIEnv *env, PRFilePrivate *priv, jthrowable excep)
> {
>-    PR_ASSERT(priv->exception == NULL);
>+    if (priv == NULL) {
>+        return;       
>+    }
>     if( priv->exception != NULL) {
>         (*env)->DeleteGlobalRef(env, priv->exception);
>     }
>     priv->exception = excep;
> }

Is this change necessary?

> jthrowable
> JSS_SSL_getException(PRFilePrivate *priv)
> {
>-    jobject retval = priv->exception;
>-    priv->exception = NULL;
>+    jobject retval = NULL;
>+    if (priv != NULL && priv->exception != NULL) {
>+        retval = priv->exception;
>+        priv->exception = NULL;
>+    }
>     return retval;
> }

Is the "priv->exception != NULL" test necessary?
Is priv->exception likely to be NULL?

Comment 6

[glen beasley]

*** This bug has been marked as a duplicate of 282732 ***