Bug 181685 - Allow dangerous disabling of poison
Summary: Allow dangerous disabling of poison
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-16 09:13 PST by JF Bastien
Modified: 2018-01-16 21:40 PST (History)
10 users (show)

See Also:


Attachments
patch (3.19 KB, patch)
2018-01-16 09:15 PST, JF Bastien
keith_miller: review+
Details | Formatted Diff | Diff
patch (3.65 KB, patch)
2018-01-16 09:33 PST, JF Bastien
no flags Details | Formatted Diff | Diff
patch (3.51 KB, patch)
2018-01-16 13:09 PST, JF Bastien
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.25 MB, application/zip)
2018-01-16 15:04 PST, EWS Watchlist
no flags Details
patch (2.81 KB, patch)
2018-01-16 16:47 PST, JF Bastien
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (2.81 KB, patch)
2018-01-16 20:28 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2018-01-16 09:13:34 PST
Some tools such as leak detectors and such like to look at real pointers, and poisoned ones confuse them. Add a JSC option to disable poisoning, but log to the console when this is done.
Comment 1 Radar WebKit Bug Importer 2018-01-16 09:13:56 PST
<rdar://problem/36546265>
Comment 2 JF Bastien 2018-01-16 09:15:30 PST
Created attachment 331392 [details]
patch
Comment 3 Keith Miller 2018-01-16 09:26:22 PST
Comment on attachment 331392 [details]
patch

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

r=me.

> Source/JavaScriptCore/ChangeLog:3
> +        Allow dangerous disabling of poison

I like to live my life on the edge...

> Source/JavaScriptCore/runtime/Options.h:490
> +    v(bool, dangerousDisablePoison, false, Normal, "if true, all poison will be initialized to 0 which defeats some Spectre and type confusion mitigations, but allows tools such as leak detectors to function better.") \

Nit: Can we put this by the other Spectre flags? Or move the other flags into their own section.
Comment 4 JF Bastien 2018-01-16 09:33:18 PST
Created attachment 331395 [details]
patch

Move option.
Comment 5 JF Bastien 2018-01-16 13:09:30 PST
Created attachment 331421 [details]
patch

Remove logging since it's not very clear and actionable.
Comment 6 Saam Barati 2018-01-16 15:00:37 PST
Comment on attachment 331421 [details]
patch

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

> Source/JavaScriptCore/runtime/Options.h:463
> +    v(bool, dangerousDisablePoison, false, Normal, "if true, all poison will be initialized to 0 which defeats some Spectre and type confusion mitigations, but allows tools such as leak detectors to function better.") \

Why did we name it like this? I would've done:
usePoisoning
or something along those lines.

Many of our options are dangerous. I don't think it needs to be called out.
Comment 7 EWS Watchlist 2018-01-16 15:04:37 PST
Comment on attachment 331421 [details]
patch

Attachment 331421 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6098072

New failing tests:
http/tests/misc/resource-timing-resolution.html
Comment 8 EWS Watchlist 2018-01-16 15:04:38 PST
Created attachment 331435 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 9 JF Bastien 2018-01-16 16:14:25 PST
(In reply to Saam Barati from comment #6)
> Comment on attachment 331421 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331421&action=review
> 
> > Source/JavaScriptCore/runtime/Options.h:463
> > +    v(bool, dangerousDisablePoison, false, Normal, "if true, all poison will be initialized to 0 which defeats some Spectre and type confusion mitigations, but allows tools such as leak detectors to function better.") \
> 
> Why did we name it like this? I would've done:
> usePoisoning
> or something along those lines.
> 
> Many of our options are dangerous. I don't think it needs to be called out.

Because in this case we'll likely tell people to use this option if they want these tools to work, and it should be used with care. Is there a precedent for us telling people "set this option to get feature X working" when it's dangerous to do so?
Comment 10 JF Bastien 2018-01-16 16:47:58 PST
Created attachment 331441 [details]
patch

Talked to Michael, moved to the name Saam suggested. Sending to CQ.
Comment 11 Saam Barati 2018-01-16 17:00:05 PST
Comment on attachment 331441 [details]
patch

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

> Source/JavaScriptCore/runtime/JSCPoison.cpp:44
> +        if (Options::usePoisoning())
> +            return;

needs to be inverted.
Comment 12 JF Bastien 2018-01-16 20:28:29 PST
Created attachment 331457 [details]
patch

(In reply to Saam Barati from comment #11)
> Comment on attachment 331441 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331441&action=review
> 
> > Source/JavaScriptCore/runtime/JSCPoison.cpp:44
> > +        if (Options::usePoisoning())
> > +            return;
> 
> needs to be inverted.

Outch, thanks for catching that! Coding while child mostly plays next to me is a bad idea...
Comment 13 WebKit Commit Bot 2018-01-16 21:15:49 PST
The commit-queue encountered the following flaky tests while processing attachment 331457 [details]:

imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html bug 181669 (authors: cdumez@apple.com and jer.noble@apple.com)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2018-01-16 21:15:58 PST
The commit-queue encountered the following flaky tests while processing attachment 331457 [details]:

imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-bitrate.html bug 181717 (authors: cdumez@apple.com and jer.noble@apple.com)
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framerate.html bug 181718 (authors: cdumez@apple.com and jer.noble@apple.com)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2018-01-16 21:40:45 PST
Comment on attachment 331457 [details]
patch

Clearing flags on attachment: 331457

Committed r227047: <https://trac.webkit.org/changeset/227047>
Comment 16 WebKit Commit Bot 2018-01-16 21:40:46 PST
All reviewed patches have been landed.  Closing bug.