Closed Bug 469577 Opened 12 years ago Closed 11 years ago
Coverity: uninitialized variable used in cert
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
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.
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: 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.
New proposed patch, less sideeffects.
Comment on attachment 449643 [details] [diff] [review] Patch v2 Kai, Doesn't this patch have the same problem I described in comment 3?
(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...)
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
You need to log in before you can comment on or make changes to this bug.