655411 - signed/unsigned comparison in ocsp.c; risk of keeping monitor locked

Status: RESOLVED FIXED

Description

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

Created attachment 530768 [details] [diff] [review]
Fix signed/unsigned warning in ocsp.c

See attached patch. Note that OCSP_Global.maxCacheEntries might be negative.

Comment 1

[Robert Relyea]

Comment on attachment 530768 [details] [diff] [review]
Fix signed/unsigned warning in ocsp.c

r+ with a few caveats.

Good catch that OCSP_Global.maxCacheEntries can be negative. Clearly it's not going to be negative in our while loop, unless ocsp_RemoveCacheItem changes it because we have a test which bounces out on the line above, so the PR_MAX is actually redundant.

NOTE: there looks like a monitor issue if the cache is disabled or unlimited. We don't all PR_ExitMonitory(). We should fix that while we are here..

bob

Comment 2

[Kai Engert (:kaie:)]

Brian, I agree the type cast is good.

But I think PR_MAX is unnecessary.

Before we arrive at that line, 
we have already checked that OCSP_Global.maxCacheEntries is positive.

We hold the lock that protects this variable, so it cannot
become negative while we are executing the loop.

Comment 3

[Kai Engert (:kaie:)]

Created attachment 543482 [details] [diff] [review]
Patch v2

Comment 4

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

I think this should go into 3.12.11 because of the monitor issue that Kai's patch fixes.

Comment 5

[Robert Relyea]

Comment on attachment 543482 [details] [diff] [review]
Patch v2

r+ rrelyea

Comment 6

[Kai Engert (:kaie:)]

3.12 branch:
Checking in ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.65.2.1; previous revision: 1.65


trunk:
Checking in ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.66; previous revision: 1.65