Bug 181685 – Allow dangerous disabling of poison

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

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.

[Radar WebKit Bug Importer]

<rdar://problem/36546265>

[JF Bastien]

Created attachment 331392 [details]
patch

[Keith Miller]

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.

[JF Bastien]

Created attachment 331395 [details]
patch

Move option.

[JF Bastien]

Created attachment 331421 [details]
patch

Remove logging since it's not very clear and actionable.

[Saam Barati]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[JF Bastien]

(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?

[JF Bastien]

Created attachment 331441 [details]
patch

Talked to Michael, moved to the name Saam suggested. Sending to CQ.

[Saam Barati]

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.

[JF Bastien]

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...

[WebKit Commit Bot]

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: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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: [e-mail] and [e-mail])
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framerate.html bug 181718 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 331457 [details]
patch

Clearing flags on attachment: 331457

Committed r227047: <https://trac.webkit.org/changeset/227047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.