Closed
Bug 1073747
Opened 9 years ago
Closed 8 years ago
nsIURLParser crashes when passed a null pointer
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: vshalimhr, Assigned: mcmanus)
Details
Attachments
(3 files, 1 obsolete file)
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?
![]() |
||
Updated•9 years ago
|
Component: General → Networking
Assignee | ||
Comment 1•9 years ago
|
||
Your results will be at https://tbpl.mozilla.org/?tree=Try&rev=2f1d75be01ae
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8496459 -
Flags: review?(sworkman)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 3•9 years ago
|
||
thanks peter
Comment 4•8 years ago
|
||
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+
Reporter | ||
Comment 5•8 years ago
|
||
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).
Reporter | ||
Comment 6•8 years ago
|
||
This one tests all 3 implementation classes: "auth=maybe", "auth=no", "auth=yes".
Attachment #8497996 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
> 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
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1537d250e6b6
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1537d250e6b6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•