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

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[arno renevier]

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.

Comment 1

[arno renevier]

Attachment: patch

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

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 3

[arno renevier]

(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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[arno renevier]

Attachment: patch v2

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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

Comment 7

[arno renevier]

Attachment: 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.

Comment 8

[Benjamin Smedberg]

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?

Comment 9

[arno renevier]

Attachment: patch v2.2

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/450e4d9ea2d5

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

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

[:Gavin Sharp [email: [e-mail]]]

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