Closed Bug 469577 Opened 12 years ago Closed 11 years ago

Coverity: uninitialized variable used in cert_ProcessOCSPResponse

Categories

(NSS :: Libraries, defect, P2)

3.12
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: KaiE)

References

Details

(Keywords: coverity)

Attachments

(1 file, 2 obsolete files)

Reported by Coverity, Firefox run 278, CID: 1393
In file nss/lib/certhigh/ocsp.c, function cert_ProcessOCSPResponse, 
it is possible to reach this line (near the bottom) 

5006             *cacheUpdateStatus = rv_cache;

with variable rv_cache uninitialized, unassigned.
Assignee: nobody → kaie
Priority: -- → P2
Keywords: coverity
Target Milestone: 3.12.3 → 3.12.5
This bug may have been fixed by now, but if not, it's a good first bug.
Attached patch Patch V 1 (obsolete) — Splinter Review
Nelson :

 I am setting cacheUpdateStatus with the variable 'rv_cache' after 'rv_cache' is assigned becuase I was not sure whether it needs to be initialized with SECStatus or SECFailure.

Regards,
Shailendra
Attachment #430913 - Flags: review?(nelson)
Comment on attachment 430913 [details] [diff] [review]
Patch V 1

This patch has the effect that we will only update 
*cacheUpdateStatus iff OCSP_Global.maxCacheEntries >= 0.
I think that's incorrect behavior.  I think we must update
it regardless of the values of OCSP_Global.maxCacheEntries.
I do not know what value we should use when 
OCSP_Global.maxCacheEntries == 0.  
Let's ask Alexei.
Attachment #430913 - Flags: review?(nelson) → review-
Alexei, please review this bug's comments and advise.
Target Milestone: 3.12.5 → Future
Target Milestone: Future → ---
I propose to use a simpler patch that avoids the sideeffect, and avoids to have the discussion that Nelson starts.

Let's simply init rv_cache to a good default value, to make coverity happy.

The access to OCSP_Global.maxCacheEntries and to the cache must happen inside the monitor.

If we don't update the cache, there is no reason to change/update that status.

Furthermore, as of today, I can't find any existing consumer of the cacheUpdateStatus result value.
Attached patch Patch v2 (obsolete) — Splinter Review
New proposed patch, less sideeffects.
Attachment #430913 - Attachment is obsolete: true
Attachment #449643 - Flags: review?(nelson)
Comment on attachment 449643 [details] [diff] [review]
Patch v2

Kai, 
Doesn't this patch have the same problem I described in comment 3?
Attached patch Patch v3Splinter Review
(In reply to comment #7)
> (From update of attachment 449643 [details] [diff] [review])
> Kai, 
> Doesn't this patch have the same problem I described in comment 3?

Ok, you're absolutly right. Sorry.


I propose the following simple patch.
If we don't update the cache, then a "cache update failure" is impossible, and I think it's ok to return status "success". (We successfully decided that the cache is fine and doesn't need an update, because the cache is disabled...)
Attachment #449643 - Attachment is obsolete: true
Attachment #449666 - Flags: review?(nelson)
Attachment #449643 - Flags: review?(nelson)
Comment on attachment 449666 [details] [diff] [review]
Patch v3

r=nelson
Attachment #449666 - Flags: review?(nelson) → review+
Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.65; previous revision: 1.64
done
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 560266
You need to log in before you can comment on or make changes to this bug.