nsIURLParser crashes when passed a null pointer

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vshalimhr, Assigned: mcmanus)

Tracking

Other Branch
mozilla35
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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?
Component: General → Networking
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
thanks peter
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.
Attachment #8496459 - Flags: review?(sworkman) → review+
Posted file test_1073747.zip (obsolete) —
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).
Posted file test_1073747.zip
This one tests all 3 implementation classes: "auth=maybe", "auth=no", "auth=yes".
Attachment #8497996 - Attachment is obsolete: true
> 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
https://hg.mozilla.org/mozilla-central/rev/1537d250e6b6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.