Multiple Locking problems in ssl3_CompressMACEncryptRecord & ssl3_HandleRecord

ASSIGNED
Unassigned

Status

ASSIGNED
8 years ago
2 years ago

People

(Reporter: briansmith, Unassigned)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.55 Safari/533.4
Build Identifier: 

1. Because both of these functions modify data in the read or write spec, they need to acquire the spec write lock. However, both functions only acquire the spec read lock instead.

2. ssl3_HandleRecord releases the spec lock too early; immediately after releasing the lock, it continues to use the crSpec in order to decompress the data. The release of the lock needs to done afterwards.

3. If decompression fails, then ssl3_CompressMACEncryptRecord returns without releasing the spec lock.

Reproducible: Always
Comment on attachment 450883 [details] [diff] [review]
Proposed fixes.

Thanks for the patch.  There are several patches for various different problems presently awaiting review (or recently reviewed) that all touch this same code (especially the label spec_locked_loser).  They must all be reconciled.  I'll review this with the others.
Attachment #450883 - Flags: review?(nelson)
Attachment #450883 - Attachment is obsolete: true
Attachment #450883 - Flags: review?(nelson)
Attachment #450883 - Attachment is obsolete: false
Created attachment 450898 [details] [diff] [review]
Fix another case where a lock needs to be released

When I moved the release down in ssl3_HandleRecord, I missed an error condition in which I needed to add lock release. This patch fixes that new problem and the pre-existing ones.
Attachment #450883 - Attachment is obsolete: true
Assignee: nobody → brian
Version: unspecified → trunk
Comment on attachment 450898 [details] [diff] [review]
Fix another case where a lock needs to be released

Brian, thanks for your patches.  Whenever you submit an NSS patch, please request review for it.  (Let me know if you cannot.) For patches to libSSL, please ask me for review.  Likewise for lib/freebl.  
For patches to lib/softken and lib/pk11wrap, ask rrelyea.
For libpkix, ask Alexei.volkov.  For other parts, you can ask any of us.
Attachment #450898 - Flags: review?(nelson)
Nelson, thanks for the information.

Almost all of my pending patches conflict with this one. If this one can get committed or rejected then I can submit patches for all the bugs I have marked as a dependency of this bug. If there's some reason that this patch can't get committed before the other bugs need to be fixed, then I will rewrite those other patches to use the spec read lock instead.

Updated

8 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
No longer blocks: 571795
No longer blocks: 571732

Comment 6

6 years ago
What is missing for this bug, a review flag?
Assignee: brian → nobody
You need to log in before you can comment on or make changes to this bug.