Closed Bug 751387 Opened 10 years ago Closed 7 years ago

"xpcshell -e" crashes [@ ProcessArgs]

Categories

(Testing :: General, defect)

x86_64
macOS
defect
Not set
critical

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jruderman, Assigned: hiro)

References

Details

(Keywords: crash)

Attachments

(1 file, 2 obsolete files)

I guess it tries to read the next argument, but there is no next argument.
Duplicate of this bug: 795685
Attached patch Fix (obsolete) — Splinter Review
In case of i > argc, 'length' is negative value so the loop will be spin many times.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bb9aab2917
Assignee: nobody → hiikezoe
Attachment #8575079 - Flags: review?(ahalberstadt)
Comment on attachment 8575079 [details] [diff] [review]
Fix

This makes sense to me, but I'm not familiar with this code and don't understand the underlying problem so would prefer to leave the review to someone else.
Attachment #8575079 - Flags: review?(ahalberstadt)
Comment on attachment 8575079 [details] [diff] [review]
Fix

Setting needinfo? from :bholley, module owner of xpconnect (which the patch seems to touch). Bobby, any idea who can do a review of this?
Flags: needinfo?(bobbyholley)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Created attachment 8575079 [details] [diff] [review]
> Fix
> 
> In case of i > argc, 'length' is negative value so the loop will be spin
> many times.

How do we end up with |i > argc|? I can't see how that would happen, but maybe I'm missing something.
Flags: needinfo?(bobbyholley)
(I can review the patch, btw - once comment 5 is answered please flag me)
(In reply to Bobby Holley (:bholley) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > Created attachment 8575079 [details] [diff] [review]
> > Fix
> > 
> > In case of i > argc, 'length' is negative value so the loop will be spin
> > many times.
> 
> How do we end up with |i > argc|? I can't see how that would happen, but
> maybe I'm missing something.

A few lines above the patch:

968     for (i = 0; i < argc; i++) {
969         if (argv[i][0] != '-' || argv[i][1] == '\0') {
970             ++i;
971             break;
972         }
973         switch (argv[i][1]) {
974           case 'v':
975           case 'f':
976           case 'e':
977             ++i;
978             break;

'i' is just argc+1 here if the argument only '-e' or others.
Attachment #8575079 - Flags: review?(bobbyholley)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> 'i' is just argc+1 here if the argument only '-e' or others.

Hm, how so? In that case argc would be 1, and we'd hit the switch statement, which would increment i from 0 to 1, and then break out of the loop, leaving us with |i == argc|, right?
(In reply to Bobby Holley (:bholley) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > 'i' is just argc+1 here if the argument only '-e' or others.
> 
> Hm, how so? In that case argc would be 1, and we'd hit the switch statement,
> which would increment i from 0 to 1, and then break out of the loop,

Unfortunately the 'break' there is against switch statement, not for loop.
The explanation in comment #7 was bad.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> (In reply to Bobby Holley (:bholley) from comment #5)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > > Created attachment 8575079 [details] [diff] [review]
> > > Fix
> > > 
> > > In case of i > argc, 'length' is negative value so the loop will be spin
> > > many times.
> > 
> > How do we end up with |i > argc|? I can't see how that would happen, but
> > maybe I'm missing something.
> 
> A few lines above the patch:
> 
> 968     for (i = 0; i < argc; i++) {
> 969         if (argv[i][0] != '-' || argv[i][1] == '\0') {
> 970             ++i;
> 971             break;
> 972         }
> 973         switch (argv[i][1]) {
> 974           case 'v':
> 975           case 'f':
> 976           case 'e':
> 977             ++i;
> 978             break;
> 
> 'i' is just argc+1 here if the argument only '-e' or others.

Actually i euqals still argc here, i will be argc+1 on the next increment of the for loop.
Oh I see. So |i| is supposed to be tracking the position of the first argument that is neither a -flag nor the argument immediately following -e, -f, or -v. And the problem is that the code assumes that _if_ -e, -f, or -v is passed, there will be an option following it.

So the attached patch works. But it seems like there are a few things we could do to clear up the situation:

* Give |i| a better name.
* add a comment explaining what's going on
* Stop re-using |i| as the loop variable in the last |for| loop.
* Replace the switch with something like the following:

bool isPairedFlag = argv[i][0] != '\0' && (argv[i][1] == 'v' || argv[i][1] == 'f' || argv[i][1] == 'e');
if (isPairedFlag && i < argc - 1) {
  ++i; // Skip over the 'foo' portion of |-v foo|, |-f foo|, or |-e foo|.
}

This solves the crash in this bug, the potential out-of-bounds read of argv[i][1], and makes things a lot clearer too.

Would you mind doing that?
Sure I will do it.
Attachment #8575079 - Attachment is obsolete: true
Attachment #8575079 - Flags: review?(bobbyholley)
Attached patch Fix crash v2 (obsolete) — Splinter Review
Addressed all comment in #11.

I renamed |i| to |rootPosition| but I am not confident it's suitable in this context because I am not familiar with js implementation.
Attachment #8575757 - Flags: review?(bobbyholley)
Comment on attachment 8575757 [details] [diff] [review]
Fix crash v2

Review of attachment 8575757 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +964,5 @@
>       * before processing any -f options, which must interleave properly with
>       * -v and -w options.  This requires two passes, and without getopt, we'll
>       * have to keep the option logic here and in the second for loop in sync.
> +     * First of all, find out the first argument position which will be passed
> +     * as scriptfile to be executed.

"as a script file"
Attachment #8575757 - Flags: review?(bobbyholley) → review+
Attached patch Fix crash v3Splinter Review
Carrying over review+.
Attachment #8575757 - Attachment is obsolete: true
Attachment #8576515 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6604fd30bd42
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.