nsIOService::SpeculativeConnectInternal crashes when given a null URI

RESOLVED FIXED in Firefox 56

Status

()

--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: florian, Assigned: schien)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

nsIOService::SpeculativeConnectInternal misses a null check on its aURI parameter, and calling this code:
Services.io.QueryInterface(Ci.nsISpeculativeConnect).speculativeConnect(null, null);
crashes at http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/netwerk/base/nsIOService.cpp#1829

Of course no front-end code would intentionally attempt to speculatively connect to a null uri, but it could happen as an accident if a newURI call in a try block throws due to an invalid url.
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8879094 [details]
Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

https://reviewboard.mozilla.org/r/150438/#review155044

::: netwerk/base/nsIOService.cpp:1830
(Diff revision 1)
>                                          nsIInterfaceRequestor *aCallbacks,
>                                          bool aAnonymous)
>  {
> +    if (!aURI) {
> +        return NS_ERROR_INVALID_ARG;
> +    }

use NS_ENSURE_ARG_POINTER(aURI);
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8879094 [details]
Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

https://reviewboard.mozilla.org/r/150438/#review155052

::: netwerk/base/nsIOService.cpp:1830
(Diff revision 1)
>                                          nsIInterfaceRequestor *aCallbacks,
>                                          bool aAnonymous)
>  {
> +    if (!aURI) {
> +        return NS_ERROR_INVALID_ARG;
> +    }

Aren't we trying to prevent using NS_ENSURE_* API?
Assignee: nobody → schien
Whiteboard: [necko-active]

Comment 4

a year ago
mozreview-review
Comment on attachment 8879094 [details]
Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

https://reviewboard.mozilla.org/r/150438/#review155158

::: netwerk/base/nsIOService.cpp:1830
(Diff revision 1)
>                                          nsIInterfaceRequestor *aCallbacks,
>                                          bool aAnonymous)
>  {
> +    if (!aURI) {
> +        return NS_ERROR_INVALID_ARG;
> +    }

I did not know about that, do you remember when have we decided it?
(In reply to Dragana Damjanovic [:dragana] from comment #4)
> 
> I did not know about that, do you remember when have we decided it?

"Previously the NS_ENSURE_* macros were used for this purpose, but those macros hide return statements and should not be used in new code. (This coding style rule isn't generally agreed, so use of NS_ENSURE_* can be valid.)"

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros
This is not enforced in netwerk/* and I have seen some patches recently that use it.
Thansk @valentin for adding the reference. Yes, there are discussions about banning NS_ENSURE_* on dev-platform and it came back from time to time. I can modify my patch if we are ok with adding new code using NS_ENSURE_*. In this case we should use NS_ENSURE_ARG since aURI is a input parameter.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> Thansk @valentin for adding the reference. Yes, there are discussions about
> banning NS_ENSURE_* on dev-platform and it came back from time to time. I
> can modify my patch if we are ok with adding new code using NS_ENSURE_*. In
> this case we should use NS_ENSURE_ARG since aURI is a input parameter.

we can use  NS_ENSURE_* API for this purpose. Please update the patch.
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
Comment on attachment 8879094 [details]
Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

https://reviewboard.mozilla.org/r/150438/#review156074
Attachment #8879094 - Flags: review?(dd.mozilla) → review+

Comment 10

a year ago
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fea04e76286
add null check for nsIOService::SpeculativeConnectInternal. r=dragana

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6fea04e76286
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.