Last Comment Bug 666224 - error when checking argument boundary in nsCommandLine::GetArgument (leads to segfault)
: error when checking argument boundary in nsCommandLine::GetArgument (leads to...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla7
Assigned To: arno renevier
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-22 05:32 PDT by arno renevier
Modified: 2011-09-09 10:19 PDT (History)
3 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (952 bytes, patch)
2011-06-22 05:34 PDT, arno renevier
no flags Details | Diff | Splinter Review
patch v2 (968 bytes, patch)
2011-06-22 11:39 PDT, arno renevier
no flags Details | Diff | Splinter Review
patch v2.1 (1.74 KB, patch)
2011-06-22 13:16 PDT, arno renevier
benjamin: review-
Details | Diff | Splinter Review
patch v2.2 (1.73 KB, patch)
2011-06-24 01:47 PDT, arno renevier
benjamin: review+
Details | Diff | Splinter Review

Description arno renevier 2011-06-22 05:32:15 PDT
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 2011-06-22 05:34:50 PDT
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 ?
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-22 09:56:28 PDT
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 2011-06-22 10:02:05 PDT
(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 [:bz] 2011-06-22 10:50:30 PDT
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 2011-06-22 11:39:45 PDT
Created attachment 541131 [details] [diff] [review]
patch v2
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-22 12:34:26 PDT
(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 2011-06-22 13:16:40 PDT
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.
Comment 8 Benjamin Smedberg [:bsmedberg] 2011-06-23 14:24:45 PDT
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 2011-06-24 01:47:29 PDT
Created attachment 541631 [details] [diff] [review]
patch v2.2
Comment 10 Dão Gottwald [:dao] 2011-06-25 03:05:47 PDT
http://hg.mozilla.org/mozilla-central/rev/450e4d9ea2d5
Comment 11 Mihaela Velimiroviciu (:mihaelav) 2011-08-23 05:57:00 PDT
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: gavin@gavinsharp.com] 2011-09-09 10:19:26 PDT
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

Note You need to log in before you can comment on or make changes to this bug.