557742 - nsHttpAuthEntry::Set can use & return an uninitialized nsresult after bug 542318

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 4

[Daniel Holbert [:dholbert]]

(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.

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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

[Honza Bambas (:mayhemer)]

(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.

Comment 8

[timeless]

Attachment: with comment

Comment 9

[:Cykesiopka]

Looks like this was fixed as part of Bug 558624:
https://hg.mozilla.org/mozilla-central/diff/7a1028b9d42f/netwerk/protocol/http/nsHttpAuthCache.cpp