1374238 - nsIOService::SpeculativeConnectInternal crashes when given a null URI

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Florian Quèze [:florian]]

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 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

Review commit: https://reviewboard.mozilla.org/r/150438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150438/

Comment 2

[Dragana Damjanovic [:dragana]]

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);

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 4

[Dragana Damjanovic [:dragana]]

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?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

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

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 7

[Dragana Damjanovic [:dragana]]

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

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8879094 [details]
Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150438/diff/1-2/

Comment 9

[Dragana Damjanovic [:dragana]]

Comment on attachment 8879094 [details]
Bug 1374238 - add null check for nsIOService::SpeculativeConnectInternal.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fea04e76286
add null check for nsIOService::SpeculativeConnectInternal. r=dragana

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6fea04e76286