571699 - Multiple Locking problems in ssl3_CompressMACEncryptRecord & ssl3_HandleRecord

Status: ASSIGNED

(NSS :: Libraries, defect, P3)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Proposed fixes.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 6

[Florian Bender]

What is missing for this bug, a review flag?