600438 - Fix the locking order assertion in ssl_Get1stHandshakeLock

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch (checked in)

This continues bug 588698 comment 25.

The proposed patch fixes the assertion as I described in bug 588698 comment 31.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 479270 [details] [diff] [review]
Proposed patch (checked in)

Wan-Teh, Your patch is good, but IMO it doesn't go far enough. 
Please consider also applying this supplemental patch to the same file:

@@ -1275,5 +1274,6 @@
 #define ssl_GetSSL3HandshakeLock(ss)   \
     { if (!ss->opt.noLocks) { \
-         PORT_Assert(!ssl_HaveXmitBufLock(ss)); \
+         PORT_Assert(PZ_InMonitor((ss)->ssl3HandshakeLock) || \
+                     !ssl_HaveXmitBufLock(ss)); \
          PZ_EnterMonitor((ss)->ssl3HandshakeLock); \
       } }
@@ -1300,6 +1300,7 @@
 #define ssl_GetRecvBufLock(ss)         \
     { if (!ss->opt.noLocks) { \
-         PORT_Assert(!ssl_HaveSSL3HandshakeLock(ss)); \
-         PORT_Assert(!ssl_HaveXmitBufLock(ss)); \
+         PORT_Assert(PZ_InMonitor((ss)->recvBufLock) || \
+                     !ssl_HaveSSL3HandshakeLock(ss) && \
+                     !ssl_HaveXmitBufLock(ss)); \
          PZ_EnterMonitor((ss)->recvBufLock); \
       } }

Comment 2

[Wan-Teh Chang]

Nelson: thanks for the suggested changes.  I'm not sure if
they are correct.

My patch allows the following locking order:
  firstHandshakeLock
  recvBufLock
  ssl3HandshakeLock
  <some callback>
  firstHandshakeLock  <== recursive
because we want to allow callbacks to call libSSL functions.

Your first suggested change is:

@@ -1275,5 +1274,6 @@
 #define ssl_GetSSL3HandshakeLock(ss)   \
     { if (!ss->opt.noLocks) { \
-         PORT_Assert(!ssl_HaveXmitBufLock(ss)); \
+         PORT_Assert(PZ_InMonitor((ss)->ssl3HandshakeLock) || \
+                     !ssl_HaveXmitBufLock(ss)); \
          PZ_EnterMonitor((ss)->ssl3HandshakeLock); \
       } }

which allows the following locking order:
  ssl3HandshakeLock
  xmitBufLock
  ssl3HandshakeLock  <== recursive

This seems undesirable.

Your second suggested change is:

@@ -1300,6 +1300,7 @@
 #define ssl_GetRecvBufLock(ss)         \
     { if (!ss->opt.noLocks) { \
-         PORT_Assert(!ssl_HaveSSL3HandshakeLock(ss)); \
-         PORT_Assert(!ssl_HaveXmitBufLock(ss)); \
+         PORT_Assert(PZ_InMonitor((ss)->recvBufLock) || \
+                     !ssl_HaveSSL3HandshakeLock(ss) && \
+                     !ssl_HaveXmitBufLock(ss)); \
          PZ_EnterMonitor((ss)->recvBufLock); \
       } }

which allows the following locking orders:
  recvBufLock
  ssl3HandshakeLock
  recvBufLock
and
  recvBufLock
  xmitBufLock
  recvBufLock

The second locking order seems undesirable.  I am less
sure about the first locking order.  Do you think we should
allow that?

Comment 3

[Wan-Teh Chang]

Comment on attachment 479270 [details] [diff] [review]
Proposed patch (checked in)

Patch checked in on the NSS trunk (NSS 3.13).

Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.81; previous revision: 1.80
done