error when checking argument boundary in nsCommandLine::GetArgument (leads to segfault)

VERIFIED FIXED in mozilla7

Status

()

Core
XPCOM
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: arno renevier, Assigned: arno renevier)

Tracking

Trunk
mozilla7
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 years ago
Created attachment 541021 [details] [diff] [review]
patch

I need help writing a test for this one: how do you test a crash in a command line handler ?
(Assignee)

Updated

6 years ago
Attachment #541021 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Assignee: nobody → arno
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

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

6 years ago
Created attachment 541131 [details] [diff] [review]
patch v2
Attachment #541021 - Attachment is obsolete: true
Attachment #541021 - Flags: review?(benjamin)
(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

6 years ago
Created attachment 541156 [details] [diff] [review]
patch v2.1

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

6 years ago
Created attachment 541631 [details] [diff] [review]
patch v2.2
Attachment #541156 - Attachment is obsolete: true
Attachment #541631 - Flags: review?(benjamin)
Attachment #541631 - Flags: review?(benjamin) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/450e4d9ea2d5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
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!
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.