Bug 153491 (CVE-2016-4731) - The thing that B3 uses to describe a stack slot should not be a Value
Summary: The thing that B3 uses to describe a stack slot should not be a Value
Status: RESOLVED FIXED
Alias: CVE-2016-4731
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
: 153684 (view as bug list)
Depends on:
Blocks: 150279
  Show dependency treegraph
 
Reported: 2016-01-26 10:22 PST by Filip Pizlo
Modified: 2017-10-11 10:24 PDT (History)
13 users (show)

See Also:


Attachments
work in progress (57.35 KB, patch)
2016-01-26 11:07 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (75.72 KB, patch)
2016-01-26 12:11 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (76.59 KB, patch)
2016-01-26 12:41 PST, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-01-26 10:22:42 PST
Values can be deleted, cloned, replaced, etc.  But when a client creates a stack slot, they want something that survives independently of the Value used to get its address.
Comment 1 Filip Pizlo 2016-01-26 11:07:05 PST
Created attachment 269891 [details]
work in progress
Comment 2 Filip Pizlo 2016-01-26 12:11:49 PST
Created attachment 269904 [details]
the patch

It seems to work.
Comment 3 WebKit Commit Bot 2016-01-26 12:14:45 PST
Attachment 269904 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/CMakeLists.txt:155:  Alphabetical sorting problem. "b3/B3StackSlot.cpp" should be before "b3/B3StackSlotKind.cpp".  [list/order] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2016-01-26 12:34:05 PST
Comment on attachment 269904 [details]
the patch

Clearing r? because it's crashing.
Comment 5 Filip Pizlo 2016-01-26 12:41:09 PST
Created attachment 269906 [details]
the patch
Comment 6 WebKit Commit Bot 2016-01-26 12:43:40 PST
Attachment 269906 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/CMakeLists.txt:155:  Alphabetical sorting problem. "b3/B3StackSlot.cpp" should be before "b3/B3StackSlotKind.cpp".  [list/order] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Geoffrey Garen 2016-01-26 13:05:54 PST
Comment on attachment 269906 [details]
the patch

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

r=me

> Source/JavaScriptCore/b3/B3Procedure.cpp:98
> +    std::unique_ptr<StackSlot> slot(new StackSlot(index, byteSize, kind));

I think "auto slot = std::make_unique<StackSlot>(...)" is a slightly more idiomatic way to write this with no loss of type description.
Comment 8 Saam Barati 2016-01-26 13:06:12 PST
Comment on attachment 269906 [details]
the patch

LGTM
Comment 9 Saam Barati 2016-01-26 13:06:15 PST
Comment on attachment 269906 [details]
the patch

LGTM
Comment 10 Saam Barati 2016-01-26 13:06:16 PST
Comment on attachment 269906 [details]
the patch

LGTM
Comment 11 Saam Barati 2016-01-26 13:06:17 PST
Comment on attachment 269906 [details]
the patch

LGTM
Comment 12 Filip Pizlo 2016-01-26 13:10:48 PST
(In reply to comment #7)
> Comment on attachment 269906 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269906&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/b3/B3Procedure.cpp:98
> > +    std::unique_ptr<StackSlot> slot(new StackSlot(index, byteSize, kind));
> 
> I think "auto slot = std::make_unique<StackSlot>(...)" is a slightly more
> idiomatic way to write this with no loss of type description.

I agree.  Unfortunately, Visual Studio will claim that this is incorrect because StackSlot's constructor is private and std::make_unique is not StackSlot's friend.  Clang figures out that since Procedure is instantiating make_unique and Procedure is StackSlot's friend, make_unique should be allowed to touch StackSlot's privates.

Since permission to touch a friend's privates does not propagate through instantiations in Visual Studio, we're forced to use this workaround.
Comment 13 Filip Pizlo 2016-01-26 14:05:55 PST
Landed in http://trac.webkit.org/changeset/195620
Comment 14 Filip Pizlo 2016-02-01 11:03:35 PST
*** Bug 153684 has been marked as a duplicate of this bug. ***