Bug 152912 - Equivalence PropertyCondition needs to check the offset it uses to load the value from is not invalidOffset
Summary: Equivalence PropertyCondition needs to check the offset it uses to load the v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-08 11:40 PST by Keith Miller
Modified: 2016-01-22 11:31 PST (History)
4 users (show)

See Also:

Attachments
Patch (2.75 KB, patch)
2016-01-21 15:14 PST, Keith Miller 		no flags Details | Formatted Diff | Diff
Patch (3.28 KB, patch)
2016-01-21 16:14 PST, Keith Miller 		no flags Details | Formatted Diff | Diff
Patch (3.28 KB, patch)
2016-01-21 16:16 PST, Keith Miller 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-01-08 11:40:35 PST 
Equivalence PropertyCondition needs to check the offset it uses to load the value from is not invalidOffset
Comment 1 Keith Miller 2016-01-21 15:14:05 PST 
Created attachment 269501 [details]
Patch
Comment 2 Keith Miller 2016-01-21 15:15:57 PST 
Note that this patch does not fix https://bugs.webkit.org/show_bug.cgi?id=134641, which is still a race and is not so awesome.
Comment 3 Mark Lam 2016-01-21 15:25:00 PST 
Comment on attachment 269501 [details]
Patch

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

> Source/JavaScriptCore/tests/stress/global-property-into-variable-get-from-scope.js:5
> +load("resources/standalone-pre.js");
> +
> +noInline();
> +
> +for (i = 0; i < 100000; i++);

How does this test the above issue?
Comment 4 Keith Miller 2016-01-21 16:14:25 PST 
Created attachment 269511 [details]
Patch
Comment 5 Keith Miller 2016-01-21 16:15:17 PST 
(In reply to comment #3)
> Comment on attachment 269501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269501&action=review
> 
> > Source/JavaScriptCore/tests/stress/global-property-into-variable-get-from-scope.js:5
> > +load("resources/standalone-pre.js");
> > +
> > +noInline();
> > +
> > +for (i = 0; i < 100000; i++);
> 
> How does this test the above issue?

Added a comment that should hopefully clarify what I knew about the cause of the bug to the test.
Comment 6 Keith Miller 2016-01-21 16:16:09 PST 
Created attachment 269512 [details]
Patch
Comment 7 Mark Lam 2016-01-22 10:39:08 PST 
Comment on attachment 269512 [details]
Patch

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

r=me

> Source/JavaScriptCore/tests/stress/global-property-into-variable-get-from-scope.js:7
> +// at that point and we would attempt to access the value at an invalid offset.

nit: Maybe add a "See https://bugs.webkit.org/show_bug.cgi?id=152912." here?
Comment 8 WebKit Commit Bot 2016-01-22 11:31:16 PST 
Comment on attachment 269512 [details]
Patch

Clearing flags on attachment: 269512

Committed r195462: <http://trac.webkit.org/changeset/195462>
Comment 9 WebKit Commit Bot 2016-01-22 11:31:20 PST 
All reviewed patches have been landed.  Closing bug.