Bug 116593 - Use uninitialized register in "JIT::emit_op_neq_null" and "emit_op_eq_null"
Summary: Use uninitialized register in "JIT::emit_op_neq_null" and "emit_op_eq_null"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-21 23:25 PDT by Peter Wang
Modified: 2013-05-23 09:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.53 KB, patch)
2013-05-21 23:43 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (4.53 KB, patch)
2013-05-22 20:09 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2013-05-22 21:09 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2013-05-22 21:43 PDT, Peter Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Wang 2013-05-21 23:25:22 PDT
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  
----------------------------------------------------------------------------
Comment 1 Peter Wang 2013-05-21 23:43:28 PDT
Created attachment 202501 [details]
Patch
Comment 2 Geoffrey Garen 2013-05-22 09:25:59 PDT
Can you add a test case for this? See LayoutTests/fast/js/typeof-codegen-crash.html for an example test case.
Comment 3 Cosmin Truta 2013-05-22 09:47:43 PDT
There's a typo in ChangeLog: "cuased" --> "caused"
Comment 4 Filip Pizlo 2013-05-22 09:58:01 PDT
Comment on attachment 202501 [details]
Patch

r=me
Comment 5 Peter Wang 2013-05-22 20:09:41 PDT
Created attachment 202639 [details]
Patch
Comment 6 Peter Wang 2013-05-22 20:21:22 PDT
(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
Comment 7 Peter Wang 2013-05-22 20:24:13 PDT
(In reply to comment #5)
> Created an attachment (id=202639) [details]
> Patch

Correct a typo and supplement a test case.
Comment 8 WebKit Commit Bot 2013-05-22 20:40:25 PDT
Comment on attachment 202639 [details]
Patch

Rejecting attachment 202639 [details] from commit-queue.

mawu@blackberry.com 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.
Comment 9 Filip Pizlo 2013-05-22 20:42:26 PDT
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. :-)
Comment 10 Filip Pizlo 2013-05-22 20:42:48 PDT
(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.
Comment 11 Charles Wei 2013-05-22 20:46:32 PDT
Comment on attachment 202639 [details]
Patch

crush == > crash
Comment 12 Peter Wang 2013-05-22 20:58:41 PDT
(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.
Comment 13 Peter Wang 2013-05-22 21:09:16 PDT
Created attachment 202641 [details]
Patch
Comment 14 Filip Pizlo 2013-05-22 21:13:34 PDT
Comment on attachment 202641 [details]
Patch

Oh, oops!  I lied.  This is missing a LayoutTests/ChangeLog.  You can list me as reviewer.
Comment 15 Peter Wang 2013-05-22 21:43:50 PDT
Created attachment 202642 [details]
Patch
Comment 16 Filip Pizlo 2013-05-22 21:44:16 PDT
Comment on attachment 202642 [details]
Patch

Awesome!  Thanks for the fix and the test!
Comment 17 WebKit Commit Bot 2013-05-22 22:16:54 PDT
The commit-queue encountered the following flaky tests while processing attachment 202642 [details]:

svg/batik/paints/patternPreserveAspectRatioA.svg bug 114139 (author: zimmermann@kde.org)
svg/batik/text/textEffect3.svg bug 116521 (authors: darin@apple.com and zimmermann@kde.org)
media/track/track-remove-crash.html bug 115892 (author: eric.carlson@apple.com)
media/audio-repaint.html bug 116648 (authors: jer.noble@apple.com, pnormand@igalia.com, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2013-05-22 22:18:16 PDT
Comment on attachment 202642 [details]
Patch

Clearing flags on attachment: 202642

Committed r150569: <http://trac.webkit.org/changeset/150569>
Comment 19 WebKit Commit Bot 2013-05-22 22:18:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Geoffrey Garen 2013-05-23 09:16:33 PDT
Nice!