1073747 - nsIURLParser crashes when passed a null pointer

Status: RESOLVED FIXED

Description

[Peter B. Shalimoff]

Attachment: nsIURLParser_test.js

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 SeaMonkey/2.25
Build ID: 20140318183706

Steps to reproduce:

nsIURLParser implementation methods [1] use the NS_PRECONDITION macro which is a noop in release builds, so they will happily pass strlen() a null pointer.

var up = Components.classes["@mozilla.org/network/url-parser;1?auth=maybe"].getService(Components.interfaces.nsIURLParser);
var o1 = {}, o2 = {}, o3 = {}, o4 = {}, o5 = {}, o6 = {};
var url = null; // or undefined.
up.parseURL(url, -1, o1, o2, o3, o4, o5, o6);

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsURLParsers.cpp



Actual results:

Firefox crashes, SeaMonkey freezes/hangs.


Expected results:

Use NS_ENSURE_ARG_POINTER or NS_ENSURE_ARG instead?

Comment 1

[Patrick McManus [:mcmanus]]

Your results will be at https://tbpl.mozilla.org/?tree=Try&rev=2f1d75be01ae

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: nsurlparser xpcom methods don't validate input

Comment 3

[Patrick McManus [:mcmanus]]

thanks peter

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8496459 [details] [diff] [review]
nsurlparser xpcom methods don't validate input

Review of attachment 8496459 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Minor changes requested. r=me.

::: netwerk/base/src/nsURLParsers.cpp
@@ +51,5 @@
>                            uint32_t *schemePos, int32_t *schemeLen,
>                            uint32_t *authorityPos, int32_t *authorityLen,
>                            uint32_t *pathPos, int32_t *pathLen)
>  {
> +    NS_ENSURE_ARG_POINTER(spec);

nit: NS_WARN_IF no? http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#276
http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#30

(For the record, I think NS_ENSURE_ARG_POINTER and friends are cleaner).

::: netwerk/test/unit/test_1073747.js
@@ +1,4 @@
> +// Test From Peter B Shalimoff
> +
> +test = function(s, funcName){
> +    function Arg(){}; Arg.prototype.toString = function(){ return this.value; };

Can you add a do_print("Testing " + funcName + " with null args"); Easier for debugging later.

@@ +2,5 @@
> +
> +test = function(s, funcName){
> +    function Arg(){}; Arg.prototype.toString = function(){ return this.value; };
> +    var args = [s, -1];
> +    for (var i = 0; i < 10; ++i) { args.push(new Arg()); }

Just want to clarify here: This will create an array starting with null, -1, then a series of 0s/nulls, right?

@@ +9,5 @@
> +	up[funcName].apply(up, args);
> +	return args;
> +	} catch (x) {
> +    do_check_true(true); // make sure it throws an exception instead of crashing
> +    return x;

nit: Indentation is off here.

@@ +11,5 @@
> +	} catch (x) {
> +    do_check_true(true); // make sure it throws an exception instead of crashing
> +    return x;
> +  }
> +};

We should always get an exception here, right? So, we should have do_check_true(false); at the end of the function, after the catch block.

Comment 5

[Peter B. Shalimoff]

Attachment: test_1073747.zip

The script I attached wasn't meant to be used as a unit test but as an interactive script to play with in the console/shell, so it contains too much unnecessary stuff for a unit test. Here's 2 clean versions for use as a unit test.
(*) "_expect_anything" suffix: doesn't care about the behavior, i.e. the only thing it expects from nsIURLParser is to handle a null argument in any way and not crash on it, i.e. either by throwing an exception or by treating the argument as an empty string.
(*) "_expect_throw" suffix: expects an exception to be thrown for a null argument (current behavior with NS_ENSURE_ARG_POINTER).

Comment 6

[Peter B. Shalimoff]

Attachment: test_1073747.zip

This one tests all 3 implementation classes: "auth=maybe", "auth=no", "auth=yes".

Comment 7

[Patrick McManus [:mcmanus]]

> Just want to clarify here: This will create an array starting with null, -1,
> then a series of 0s/nulls, right?
> 
>

right - I've added some comments

Comment 8

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1537d250e6b6

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1537d250e6b6