Closed
Bug 1201229
Opened 9 years ago
Closed 9 years ago
nsIHttpChannel::GetRequest/ResponseHeader is a footgun
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(1 file)
8.39 KB,
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
See bug 1200869 as an example of the problem. These APIs don't touch the out string argument if the header doesn't exist, and instead throw an exception. This makes many C++ callers potentially unsafe since they may end up using whatever was in the string before the call. Jason, do you think we can/should fix this?
Flags: needinfo?(jduell.mcbugs)
Comment 1•9 years ago
|
||
I can't see 1180091 (cc me?) This behavior is exposed to JS currently, so I'm guessing we should be careful about breaking addsons. Would you be happier with an alternate API that writes a blank string into the argument? (Note that this then makes it ambiguous whether a header was present and blank-valued, or missing entirely).
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #1) > I can't see 1180091 (cc me?) Looks like someone beat me to it! :-) > This behavior is exposed to JS currently, so I'm guessing we should be > careful about breaking addsons. Is it though? nsHttpHeaderArray returns NS_ERROR_NOT_AVAILABLE which will propagate up to these functions, and then XPConnect converts that error into an exception, so JS never gets to see the return value. It is only internal C++ callers that we need to care about. My question was mostly regarding those callers.
Assignee | ||
Comment 3•9 years ago
|
||
I'm testing a patch on try to see what happens!
Assignee | ||
Comment 4•9 years ago
|
||
Looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11128ff2e994
Assignee | ||
Comment 5•9 years ago
|
||
This fixes nsIHttpChannel::GetRequestHeader() and nsIHttpChannel::GetResponseHeader() to always empty out their string argument even when they fail. This prevents programming mistakes of passing the same string object to multiple of these calls and using the string value without checking the nsresult error code, since otherwise the string value may be unchanged from a previous call. Note that this doesn't affect JS consumers of these APIs since we only empty out the string argument in case the method fails, which will be translated to a JS exception, and the JS code will never get to see the emptied string.
Attachment #8656550 -
Flags: review?(jduell.mcbugs)
Updated•9 years ago
|
status-firefox43:
--- → affected
tracking-firefox43:
--- → +
Assignee | ||
Comment 6•9 years ago
|
||
Hmm, Liz, why do you think this should be tracked for 43? I don't think this is a serious issue, it's more about eliminating a footgun in our code for the future.
Flags: needinfo?(lhenry)
Comment 7•9 years ago
|
||
Ehsan: I was following up from another bug and thought this sounded like something that people may want to uplift (if it gets fixed). I'll just untrack it since you think it's not important for relman to follow up. Thanks for the feedback!
tracking-firefox43:
+ → ---
Flags: needinfo?(lhenry)
Assignee | ||
Comment 8•9 years ago
|
||
I don't think we'd want to uplift this. Thanks for checking!
Comment 9•9 years ago
|
||
Comment on attachment 8656550 [details] [diff] [review] Return an empty string for a header when an error occurs Review of attachment 8656550 [details] [diff] [review]: ----------------------------------------------------------------- LGTM Jason asked me to review this bug. I thing that his comment in this bug was actually meant for bug 815299.
Attachment #8656550 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #9) > Jason asked me to review this bug. I thing that his comment in this bug was > actually meant for bug 815299. Hmm, no, why do you think that?
Comment 12•9 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #10) > (In reply to Dragana Damjanovic [:dragana] from comment #9) > > Jason asked me to review this bug. I thing that his comment in this bug was > > actually meant for bug 815299. > > Hmm, no, why do you think that? I am probably wrong. My thoughts: Because actually being able to write empty string into request header could break addons if they always expect to get a value if the call to GetRequestHeader does not throw. Also both patches independently could potentially break addons, hopefully they will not.
Assignee | ||
Comment 13•9 years ago
|
||
Oh, I see what you mean now. Yes, that bug can break add-ons at least in theory, but this one cannot (please see comment 2.)
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd8cf8418f05
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•