Closed
Bug 666224
Opened 13 years ago
Closed 13 years ago
error when checking argument boundary in nsCommandLine::GetArgument (leads to segfault)
Categories
(Core :: XPCOM, defect)
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: arno, Assigned: arno)
Details
Attachments
(1 file, 3 obsolete files)
1.73 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Hi, when trying to access cmdLine.getArgument(cmdLine.length); in a command line handle (cmdLine being a nsICommandLine), the application segfaults. I think this is caused by an error checking argument validity: NS_ENSURE_ARG_MAX(PRUint32(aIndex), mArgs.Length()); This test succeeds when aIndex is equals to mArgs.Length(), but as index is 0 based, aIndex + 1 should be tested instead.
Assignee | ||
Comment 1•13 years ago
|
||
I need help writing a test for this one: how do you test a crash in a command line handler ?
Assignee | ||
Updated•13 years ago
|
Attachment #541021 -
Flags: review?(benjamin)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → arno
Comment 2•13 years ago
|
||
Comment on attachment 541021 [details] [diff] [review] patch Can't you just write an xpcshell test that calls cmdLine.getArgument() with bogus index values? I think that harness handles crashing just fine. I think using mArgs.Length() - 1 would be more intuitive.
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to comment #2) > Comment on attachment 541021 [details] [diff] [review] [review] > patch > > Can't you just write an xpcshell test that calls cmdLine.getArgument() with > bogus index values? I think that harness handles crashing just fine. Yes, but I don't known how to access cmdLine from xpcshell > I think using mArgs.Length() - 1 would be more intuitive. Problem is: mArgs.Length() can be 0. Then mAgrs.Lenght() - 1 could be negative.
Comment 4•13 years ago
|
||
On the face of it, that patch is wrong. If aIndex == -1, then the patch will compare 0 to the length and test true no matter what the length is. You really just want to stop using NS_ENSURE_ARG_MAX and use a strict less than check.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #541021 -
Attachment is obsolete: true
Attachment #541021 -
Flags: review?(benjamin)
Comment 6•13 years ago
|
||
(In reply to comment #3) > Yes, but I don't known how to access cmdLine from xpcshell Same way you do from any other code: var cmdLine=Components.classes["@mozilla.org/toolkit/command-line;1"].getService(Components.interfaces.nsICommandLine); cmdLine.getArgument(cmdLine.length);
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to comment #6) > (In reply to comment #3) > > Yes, but I don't known how to access cmdLine from xpcshell > > Same way you do from any other code: > > var > cmdLine=Components.classes["@mozilla.org/toolkit/command-line;1"]. > getService(Components.interfaces.nsICommandLine); > cmdLine.getArgument(cmdLine.length); Thank, I just knewn how to access nsICommandLine from a registered command-line-handler handler.
Attachment #541131 -
Attachment is obsolete: true
Attachment #541156 -
Flags: review?(benjamin)
Comment 8•13 years ago
|
||
Comment on attachment 541156 [details] [diff] [review] patch v2.1 In the test, we should be using .createInstance to get the command-line instance, not .getService. And why not just keep using NS_ENSURE_ARG_MAX, but subtract one?
Attachment #541156 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #541156 -
Attachment is obsolete: true
Attachment #541631 -
Flags: review?(benjamin)
Updated•13 years ago
|
Attachment #541631 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 10•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/450e4d9ea2d5
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Comment 11•13 years ago
|
||
Can anyone please provide some guidelines that I can use to verify this fix? Do I need to create any specific environment to test it? Thanks!
Comment 12•13 years ago
|
||
1) Load tbpl.mozilla.org, look for most recent green "X" run, click it, click link to "full log" (e.g.: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1315580859.1315582141.9462.gz&fulltext=1 ) 2) Search for a "TEST-PASS" line that includes test_bug666224.js
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•