Bug 116593 – Use uninitialized register in "JIT::emit_op_neq_null" and "emit_op_eq_null"

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Peter Wang]

In JITOpcodes32_64.cpp:794 and JITOpcodes32_64.cpp:825, a statement 
     loadPtr(Address(regT2, Structure::globalObjectOffset()), regT2);
uses uninitialized register.This mistake will cause random crush.

My port is based on ARMv7, repeatedly invoking following js code will cause crush:
----------------------------------------------------------------------------
function crush()
{
    return (document.all!=null && b);
}
---------------------------------------------------------------------------- 


The gdb output
----------------------------------------------------------------------------
Program received signal SIGSEGV, Segmentation fault.
[Switching to pid 38916178 tid 3 name "webkit_main"]
0x79697bf4 in ?? ()
(gdb) bt
#0  0x79697bf4 in ?? ()
#1  0x79697c74 in ?? ()
#2  0x79697c74 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) x/50i 0x79697bc4
   0x79697bc4:	adds	r3, #90	; 0x5a
   0x79697bc6:	str	r0, [r3, #0]
   0x79697bc8:	movw	r3, #31220
   0x79697bcc:	movt	r3, #31578
   0x79697bd0:	str	r1, [r3, #0]    
   0x79697bd2:	str	r0, [r5, #0]    // Following instructions are generated by "emit_op_neq_null"
   0x79697bd4:	str	r1, [r5, #4]
   0x79697bd6:	cmn.w	r1, #5
   0x79697bda:	bne.n	0x79697c00
   0x79697bdc:	ldr	r1, [r0, #0]
   0x79697bde:	ldrb.w	r3, [r1, #53]
   0x79697be2:	tst.w	r3, #1
   0x79697be6:	bne.n	0x79697bec
   0x79697be8:	movs	r1, #1
   0x79697bea:	b.n	0x79697c16
   0x79697bec:	movw	r0, #61496
   0x79697bf0:	movt	r0, #31128
=> 0x79697bf4:	ldr	r2, [r2, #4]    // Generated by JITOpcodes32_64.cpp:825
   0x79697bf6:	cmp	r0, r2
(gdb) p/x $r2                           // The content of r2 is an invalid address
$1 = 0xfffffffb
----------------------------------------------------------------------------

Dumped bytecode
----------------------------------------------------------------------------
Source: function IsFilterNeeded() { return (document.all!=null && b); }

[   0] enter                                                                                                              
[   1] resolve           r1, document(@id0), 2067299048                                                                   
[   6] get_by_id         r0, r1, all(@id1)                                                                                
[  15] neq_null          r0, r0                                                                                           
[  18] jfalse            r0, 8(->26)                                                                                      
[  21] resolve           r0, b(@id2), 2067299060                                                                          
[  26] ret               r0                                                                                               
                                                                                                                          
Identifiers:                                                                                                              
  id0 = document                                                                                                          
  id1 = all                                                                                                               
  id2 = b  
----------------------------------------------------------------------------

[Peter Wang]

Created attachment 202501 [details]
Patch

[Geoffrey Garen]

Can you add a test case for this? See LayoutTests/fast/js/typeof-codegen-crash.html for an example test case.

[Cosmin Truta]

There's a typo in ChangeLog: "cuased" --> "caused"

[Filip Pizlo]

Comment on attachment 202501 [details]
Patch

r=me

[Peter Wang]

Created attachment 202639 [details]
Patch

[Peter Wang]

(In reply to comment #2)
> Can you add a test case for this? See LayoutTests/fast/js/typeof-codegen-crash.html for an example test case.
Thank you very much for help

[Peter Wang]

(In reply to comment #5)
> Created an attachment (id=202639) [details]
> Patch

Correct a typo and supplement a test case.

[WebKit Commit Bot]

Comment on attachment 202639 [details]
Patch

Rejecting attachment 202639 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Filip Pizlo]

Comment on attachment 202639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202639&action=review

> LayoutTests/fast/js/script-tests/neq-null-crash.js:9
> +for (var i = 1; i < 20; i++) {
> +    shouldBeFalse("crush()");
> +}

Can you change this to i < 100?  That will be enough to make the DFG JIT kick in, and this patch will automatically have way more coverage. :-)

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 202639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202639&action=review
> 
> > LayoutTests/fast/js/script-tests/neq-null-crash.js:9
> > +for (var i = 1; i < 20; i++) {
> > +    shouldBeFalse("crush()");
> > +}
> 
> Can you change this to i < 100?  That will be enough to make the DFG JIT kick in, and this patch will automatically have way more coverage. :-)

I'll be happy to cq+ once you make that change.

[Charles Wei]

Comment on attachment 202639 [details]
Patch

crush == > crash

[Peter Wang]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 202639 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202639&action=review
> > 
> > > LayoutTests/fast/js/script-tests/neq-null-crash.js:9
> > > +for (var i = 1; i < 20; i++) {
> > > +    shouldBeFalse("crush()");
> > > +}
> > 
> > Can you change this to i < 100?  That will be enough to make the DFG JIT kick in, and this patch will automatically have way more coverage. :-)
> 
> I'll be happy to cq+ once you make that change.

Ok. Thank you.

[Peter Wang]

Created attachment 202641 [details]
Patch

[Filip Pizlo]

Comment on attachment 202641 [details]
Patch

Oh, oops!  I lied.  This is missing a LayoutTests/ChangeLog.  You can list me as reviewer.

[Peter Wang]

Created attachment 202642 [details]
Patch

[Filip Pizlo]

Comment on attachment 202642 [details]
Patch

Awesome!  Thanks for the fix and the test!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 202642 [details]:

svg/batik/paints/patternPreserveAspectRatioA.svg bug 114139 (author: [e-mail])
svg/batik/text/textEffect3.svg bug 116521 (authors: [e-mail] and [e-mail])
media/track/track-remove-crash.html bug 115892 (author: [e-mail])
media/audio-repaint.html bug 116648 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 202642 [details]
Patch

Clearing flags on attachment: 202642

Committed r150569: <http://trac.webkit.org/changeset/150569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Nice!