Fix the locking order assertion in ssl_Get1stHandshakeLock

RESOLVED FIXED in 3.13

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.45 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 479270 [details] [diff] [review]
Proposed patch (checked in)

This continues bug 588698 comment 25.

The proposed patch fixes the assertion as I described in bug 588698 comment 31.
Attachment #479270 - Flags: review?(nelson)
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); \
       } }
Attachment #479270 - Flags: review?(nelson) → review+
(Assignee)

Comment 2

8 years ago
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?
(Assignee)

Comment 3

8 years ago
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
Attachment #479270 - Attachment description: Proposed patch → Proposed patch (checked in)
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.