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

RESOLVED FIXED in 3.12.11

Status

NSS
Libraries
--
trivial
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: briansmith, Unassigned)

Tracking

trunk
3.12.11

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

See attached patch. Note that OCSP_Global.maxCacheEntries might be negative.
Attachment #530768 - Flags: review?(rrelyea)

Comment 1

7 years ago
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
Attachment #530768 - Flags: review?(rrelyea) → review+

Comment 2

6 years ago
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

6 years ago
Created attachment 543482 [details] [diff] [review]
Patch v2
Attachment #543482 - Flags: review?(rrelyea)
Attachment #530768 - Attachment is obsolete: true
I think this should go into 3.12.11 because of the monitor issue that Kai's patch fixes.
Target Milestone: --- → 3.12.11

Comment 5

6 years ago
Comment on attachment 543482 [details] [diff] [review]
Patch v2

r+ rrelyea
Attachment #543482 - Flags: review?(rrelyea) → review+

Updated

6 years ago
Summary: signed/unsigned comparison in ocsp.c → signed/unsigned comparison in ocsp.c; risk of keeping monitor locked

Comment 6

6 years ago
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.