Closed
Bug 557742
Opened 14 years ago
Closed 9 years ago
nsHttpAuthEntry::Set can use & return an uninitialized nsresult after bug 542318
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking1.9.2 | --- | needed |
status1.9.2 | --- | wanted |
status1.9.1 | --- | unaffected |
People
(Reporter: dholbert, Assigned: dougt)
References
()
Details
(Keywords: regression)
Attachments
(1 file)
1.48 KB,
patch
|
Biesinger
:
review+
|
Details | Diff | Splinter Review |
Just noticed this build warning in an opt build: > netwerk/protocol/http/src/nsHttpAuthCache.cpp:418: warning: ‘rv’ may be used uninitialized in this function Before bug 542318's patch[1], this function immediately initialized the variable |rv|. However, bug 542318's patch that immediate-initialization, and left it open to being checked & returned while still-uninitialized under certain circumstances. Code snippet: 418 nsresult rv; 419 if (ident) { 420 rv = mIdent.Set(*ident); 421 } 422 else if (mIdent.IsEmpty()) { [...] 427 rv = mIdent.Set(nsnull, nsnull, nsnull); 428 } 429 if (NS_FAILED(rv)) { 430 free(newRealm); 431 return rv; 432 } If we fail the "if" and the "else if" checks, then we end up using an uninitialized value at line 429 and 431. We probably just want to set rv to NS_OK when we declare it, right? (or add a final "else" clause at line 428, with |rv = NS_OK;| BTW, this affects 1.9.2 as well, since bug 542318 was backported to that branch.
Reporter | ||
Updated•14 years ago
|
Blocks: 542318
Summary: nsHttpAuthEntry::Set can return uninitialized value after bug 542318 → nsHttpAuthEntry::Set can use & return an uninitialized nsresult after bug 542318
Reporter | ||
Comment 1•14 years ago
|
||
(In reply to comment #0) > Before bug 542318's patch[1] [1] http://hg.mozilla.org/mozilla-central/diff/ab20dd6c7d09/netwerk/protocol/http/src/nsHttpAuthCache.cpp
Comment 2•14 years ago
|
||
Like I said in bug 557742, it really doesn't seem to me like not changing mIdent in that case is the right thing to do.
Reporter | ||
Comment 4•14 years ago
|
||
(In reply to comment #2) > Like I said in bug 557742 That's this bug :) I think you mean to say Bug 556335 comment 2.
Reporter | ||
Comment 5•14 years ago
|
||
Requesting blocking1.9.2, since this is a case of us introducing random behavior (dependent on an uninitialized value) into the 1.9.2 branch.
blocking1.9.2: --- → ?
Updated•14 years ago
|
blocking1.9.2: ? → needed
Updated•14 years ago
|
Keywords: regression
Reporter | ||
Comment 6•14 years ago
|
||
To clarify biesi's comment 2: he's saying that a simple "|rv = NS_OK;|" tweak (as suggested in comment 0) would be incorrect/insufficient. Honza, would you mind taking a look at this?
Comment 7•14 years ago
|
||
(In reply to comment #6) > To clarify biesi's comment 2: he's saying that a simple "|rv = NS_OK;|" tweak > (as suggested in comment 0) would be incorrect/insufficient. I'd rather say he complains about not changing non-empty mIdent when there is 'ident' argument null. However, this behavior is core fix for bug 542318, to do not overwrite cached identity when there is not a valid identity available in the consumer (nsHttpChannel). Bug 542318 comment 32 explains the issues. > > Honza, would you mind taking a look at this? I am using this simple and transparent fix (to do not change mIdent when ident is null) rather then adding some new flags and complex logic to http channel handling m(Proxy)Ident, cache and identityInvalid flag states. Just adding final else {rv=NS_OK;} is IMHO ok. BTW thanks for the catch.
Status: NEW → ASSIGNED
QA Contact: networking.http → honzab.moz
Updated•14 years ago
|
Assignee: nobody → honzab.moz
QA Contact: honzab.moz → networking.http
Assignee: honzab.moz → timeless
Attachment #444078 -
Flags: review?(cbiesinger)
Updated•14 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → wanted
Updated•13 years ago
|
Attachment #444078 -
Flags: review?(cbiesinger) → review+
Comment 9•9 years ago
|
||
Looks like this was fixed as part of Bug 558624: https://hg.mozilla.org/mozilla-central/diff/7a1028b9d42f/netwerk/protocol/http/nsHttpAuthCache.cpp
Assignee: timeless → dougt
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Depends on: 558624
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•