751387 - "xpcshell -e" crashes [@ ProcessArgs]

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jesse Ruderman]

I guess it tries to read the next argument, but there is no next argument.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

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

Comment 3

[Andrew Halberstadt [:ahal]]

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.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

(I can review the patch, btw - once comment 5 is answered please flag me)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 11

[Bobby Holley (:bholley)]

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?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Sure I will do it.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix crash v2

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.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Pushed try now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e60aadf73121

Comment 15

[Bobby Holley (:bholley)]

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"

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix crash v3

Carrying over review+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6604fd30bd42

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6604fd30bd42