Closed Bug 1031682 Opened 6 years ago Closed 6 years ago

LRecoverInfo::OperandIter is crashing when iterate over a MNode without operand

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: caiolima, Assigned: caiolima)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 5 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

Apply the patch https://bugzilla.mozilla.org/attachment.cgi?id=8446946 (RArgumentsLength) and run the script https://bugzilla.mozilla.org/attachment.cgi?id=8443471


Actual results:

The JS engine crashed on running and returned this error:
js engine is returning this following error:

Hit MOZ_CRASH(indexing into zero-length array) at ../../dist/include/mozilla/Array.h:49
[New Thread 0x170b of process 16391]
[New Thread 0x1803 of process 16391]
[New Thread 0x1903 of process 16391]
[New Thread 0x1a03 of process 16391]
[New Thread 0x1b03 of process 16391]

Catchpoint 1 (signal SIGSEGV), 0x0000000100518ad2 in mozilla::Array<js::jit::MUse, 0ul>::operator[](unsigned long) const ()


Expected results:

The JS Engine should end processing the script without error and the correct execution.
Blocks: 1024609
Whiteboard: [lang=c++]
OS: Mac OS X → All
Hardware: x86 → All
Attached patch 1031682.patch (obsolete) — Splinter Review
I've tried to do it on OperandIter constructor, but It was not working because of some it passed as parameters aren't valid accessible mem addresses. This bug was not present only on TotalOperandsCount, but on ohter places, So I've diceded to call settle() over the operands.

There is no problem on vector having only zero arguments nodes, because they have a topological sort, and the last one is a Resume Point, a noz-zero MNode.

By the way, I've already pushed it to tpbl:
https://tbpl.mozilla.org/?tree=Try&rev=771957d0ca66
Attachment #8447704 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8447704 [details] [diff] [review]
1031682.patch

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

Have a look at other settle functions [1].

[1] http://dxr.mozilla.org/mozilla-central/search?q=settle%28%29&case=false

::: js/src/jit/LIR.h
@@ +973,5 @@
>          { }
>  
> +        void settle() {
> +            while ((*it_)->numOperands() == 0) {
> +                ++it_;

This function is good.

@@ +988,1 @@
>              return (*it_)->getOperand(op_);

The goal of these operator is to access the value, not to settle on something.  The goal of the constructor and the operator++ is to find the next read-able operand, not the operator* nor the operator->, which are only to access the operand.

@@ +990,5 @@
>  
>          OperandIter &operator ++() {
>              ++op_;
> +
> +            settle();

The operator++ must call the settle function, but this is not the right location for doing so.
Attachment #8447704 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 8447704 [details] [diff] [review]
> 1031682.patch
> 
> Review of attachment 8447704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have a look at other settle functions [1].
> 
> [1] http://dxr.mozilla.org/mozilla-central/search?q=settle%28%29&case=false
> 
> ::: js/src/jit/LIR.h
> @@ +973,5 @@
> >          { }
> >  
> > +        void settle() {
> > +            while ((*it_)->numOperands() == 0) {
> > +                ++it_;
> 
> This function is good.
> 
> @@ +988,1 @@
> >              return (*it_)->getOperand(op_);
> 
> The goal of these operator is to access the value, not to settle on
> something.  The goal of the constructor and the operator++ is to find the
> next read-able operand, not the operator* nor the operator->, which are only
> to access the operand.

I Agree with you. I've done this way, because placing the settle() on constructor is performing segmentation fault, due some MNode **it values are not accessible memory values. I've identified that it's occurring due the OperandIter is created for the recoverInfo->end()[1];

I'm thinking on change the usage of 2 OperandIter and use only one with a new method hasNextNode(), because the |end Iterator| never iterates and this approach solves the problem I've mentioned above. What do you think? 

> @@ +990,5 @@
> >  
> >          OperandIter &operator ++() {
> >              ++op_;
> > +
> > +            settle();
> 
> The operator++ must call the settle function, but this is not the right
> location for doing so.

No problem, I'm going to place it one line before the return.

[1] - dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR.cpp?from=LIR.cpp#185

Please, remember to confirm this issue and to assign it to me. =)
(In reply to Caio Lima(:caiolima) from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > Comment on attachment 8447704 [details] [diff] [review]
> > 1031682.patch
> > 
> > Review of attachment 8447704 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Have a look at other settle functions [1].
> > 
> > [1] http://dxr.mozilla.org/mozilla-central/search?q=settle%28%29&case=false
> > 
> > ::: js/src/jit/LIR.h
> > @@ +973,5 @@
> > >          { }
> > >  
> > > +        void settle() {
> > > +            while ((*it_)->numOperands() == 0) {
> > > +                ++it_;
> > 
> > This function is good.
> > 
> > @@ +988,1 @@
> > >              return (*it_)->getOperand(op_);
> > 
> > The goal of these operator is to access the value, not to settle on
> > something.  The goal of the constructor and the operator++ is to find the
> > next read-able operand, not the operator* nor the operator->, which are only
> > to access the operand.
> 
> I Agree with you. I've done this way, because placing the settle() on
> constructor is performing segmentation fault, due some MNode **it values are
> not accessible memory values. I've identified that it's occurring due the
> OperandIter is created for the recoverInfo->end()[1];

Then can we avoid calling settle when it_ is invalid?

> I'm thinking on change the usage of 2 OperandIter and use only one with a
> new method hasNextNode(), because the |end Iterator| never iterates and this
> approach solves the problem I've mentioned above. What do you think? 

I am not sure to understand what you mean with "change the usage of 2 OperandIter".
Assignee: nobody → ticaiolima
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Then can we avoid calling settle when it_ is invalid?

There is no only one case see the outputs [1] and [2]. On [2], I've checked if *it_ != 0.
 
> > I'm thinking on change the usage of 2 OperandIter and use only one with a
> > new method hasNextNode(), because the |end Iterator| never iterates and this
> > approach solves the problem I've mentioned above. What do you think? 
> 
> I am not sure to understand what you mean with "change the usage of 2
> OperandIter".

As you can see on [3], there are 2 OperandIter objects, one is poiting to the current node and is being iterated, and other to mark the end. I don't like this approach, since it can be implemented using only one OperandIter that holds both pointers and have a method hasNextNode/hasNextOperand. I guess that it's more consistent and more intuitive to understand the code.  

[1] - https://pastebin.mozilla.org/5503336
[2] - https://pastebin.mozilla.org/5503342
[3] - http://dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR.cpp?from=LIR.cpp#185
Attached patch 1031682_skip_non_valid.patch (obsolete) — Splinter Review
This is the patch that I've used to generate the above results.
Comment on attachment 8448699 [details] [diff] [review]
1031682_skip_non_valid.patch

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

This sounds good, modify your patch accordingly (add r=nbp to the commit message), attach a fixed version of this patch, and send it to try (with a limited set of platforms).
Then when everything is green on Try, set the flag checkin-needed in the keywords of this bug such as a sheriff can land this patch for you ;)

::: js/src/jit/LIR.cpp
@@ +178,5 @@
>      dump(stderr);
>  }
>  
> +void
> +LRecoverInfo::OperandIter::settle() {

style-nit: move the open brace on the next line.

@@ +182,5 @@
> +LRecoverInfo::OperandIter::settle() {
> +
> +    if (*it_ == 0) {
> +        return;
> +    }

style-nit:
    if (!*it_)
        return;

::: js/src/jit/LIR.h
@@ +984,5 @@
>          }
>  
>          OperandIter &operator ++() {
>              ++op_;
> +

nit: remove useless blank line.

@@ +996,1 @@
>              return *this;

nit: blank-line overload.
Attachment #8448699 - Flags: review+
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> @@ +996,1 @@
> >              return *this;
> 
> nit: blank-line overload.

Click on the review link to get more context … ( :( )
Attachment #8447704 - Attachment is obsolete: true
I'll need to change some other f(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Comment on attachment 8448699 [details] [diff] [review]
> 1031682_skip_non_valid.patch
> 
> Review of attachment 8448699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This sounds good, modify your patch accordingly (add r=nbp to the commit
> message), attach a fixed version of this patch, and send it to try (with a
> limited set of platforms).
> Then when everything is green on Try, set the flag checkin-needed in the
> keywords of this bug such as a sheriff can land this patch for you ;)

Actually, this patch is not working yet. It's the patch that is resulting the [1].

[1] - https://pastebin.mozilla.org/5503342

(In reply to Caio Lima(:caiolima) from comment #5)
> As you can see on [3], there are 2 OperandIter objects, one is poiting to
> the current node and is being iterated, and other to mark the end. I don't
> like this approach, since it can be implemented using only one OperandIter
> that holds both pointers and have a method hasNextNode/hasNextOperand. I
> guess that it's more consistent and more intuitive to understand the code.  
> 
> [1] - https://pastebin.mozilla.org/5503336
> [2] - https://pastebin.mozilla.org/5503342
> [3] -
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR.cpp?from=LIR.
> cpp#185

What do you think about it?
Attached patch 1031682.patch (obsolete) — Splinter Review
I've implemented the settle based on what we discussed above.

the tbpl running is https://tbpl.mozilla.org/?tree=Try&rev=2061c78cab59
Attachment #8448699 - Attachment is obsolete: true
Attachment #8449130 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8449130 [details] [diff] [review]
1031682.patch

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

With the suggestion made previously, there is room for style improvement.
Also, this patch has a bug (see comments below).

::: js/src/jit/LIR.h
@@ +969,5 @@
>          size_t op_;
>  
>        public:
> +        explicit OperandIter(MNode **it, MNode **end)
> +          : it_(it), end_(end), op_(0)

Replace both operands by a LRecoverInfo *, from which you read the begin() and end() function and store the results in it_(…) and end_(…).

@@ +995,5 @@
>                  op_ = 0;
>                  ++it_;
>              }
> +            if (!this) {
> +                settle();

This condition is wrong, as !this does not call the operator bool() that is defined below, but it cast the pointer type to a boolean, which would always be true, even if we reached the last MNode.  You want to check "!*this" instead.

style-nit: remove the braces.

@@ +1008,2 @@
>          bool operator !=(const OperandIter &where) const {
>              return it_ != where.it_ || op_ != where.op_;

Remove this function if it is not used.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +317,5 @@
>  #endif
>  
>      uint32_t allocIndex = 0;
> +    LRecoverInfo::OperandIter it(recoverInfo->begin(), recoverInfo->end());
> +    for (; !it; ++it) {

Move the LRecoverInfo::OperandIter it(recoverInfo)  inside the init-part of the for loop.

::: js/src/jit/shared/Lowering-shared.cpp
@@ +106,5 @@
>          return nullptr;
>  
>      size_t index = 0;
> +    LRecoverInfo::OperandIter it(recoverInfo->begin(), recoverInfo->end());
> +    for (; !it; ++it) {

same here.

@@ +165,5 @@
>          return nullptr;
>  
>      size_t index = 0;
> +    LRecoverInfo::OperandIter it(recoverInfo->begin(), recoverInfo->end());
> +    for (; !it; ++it) {

and here.
Attachment #8449130 - Flags: review?(nicolas.b.pierron) → feedback+
(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> @@ +995,5 @@
> >                  op_ = 0;
> >                  ++it_;
> >              }
> > +            if (!this) {
> > +                settle();
> 
> This condition is wrong, as !this does not call the operator bool() that is
> defined below, but it cast the pointer type to a boolean, which would always
> be true, even if we reached the last MNode.  You want to check "!*this"
> instead.

Note that due to this bug, the settle function is only called from the constructor and never from the operator++, which solve the issue you have with MArgumentsLength, as it is registered as the first recover instruction in the test case.
(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> (In reply to Nicolas B. Pierron [:nbp] from comment #11)
> > @@ +995,5 @@
> > >                  op_ = 0;
> > >                  ++it_;
> > >              }
> > > +            if (!this) {
> > > +                settle();
> > 
> > This condition is wrong, as !this does not call the operator bool() that is
> > defined below, but it cast the pointer type to a boolean, which would always
> > be true, even if we reached the last MNode.  You want to check "!*this"
> > instead.
> 
> Note that due to this bug, the settle function is only called from the
> constructor and never from the operator++, which solve the issue you have
> with MArgumentsLength, as it is registered as the first recover instruction
> in the test case.

Thank you for this warn, I did not realize this bug!
Attached patch 1031682.patch (obsolete) — Splinter Review
Implemented the tips and fixed the bug reported by nbp.
Attachment #8449474 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8449474 [details] [diff] [review]
1031682.patch

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

1. Change your patch subject to fit the commit guideline [1].
2. Fix the nits.
3. Upload a new version and carry the r+ over the new patch.
4. Send your change to the try server.
5. Set the checkin-needed keyword when the try push is green.
6. Wait for a sheriff to land your patch.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

::: js/src/jit/LIR.cpp
@@ +180,5 @@
>  
>  static size_t
>  TotalOperandCount(LRecoverInfo *recoverInfo)
>  {
> +

style-nit: remove this extra line.
Attachment #8449474 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8449130 - Attachment is obsolete: true
Attached patch 1031682.patch (obsolete) — Splinter Review
Nit and patch message changed.
Attachment #8449474 - Attachment is obsolete: true
Attachment #8449494 - Flags: review+
Comment on attachment 8449494 [details] [diff] [review]
1031682.patch

> Bug 1031682 - Added a settle method on OperandIter to skip MNode(s) with zero operand and embedded the end on OperandIter object. r=rbp

Shorter would be nicer:

Bug 1031682 - Add OperandIter::settle to skip instructions with no operands. r=nbp
Should I restart the try because of message change?
Attached patch 1031682.patchSplinter Review
Attachment #8449494 - Attachment is obsolete: true
Attachment #8449502 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/af2c77766265
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.