Bug 181483 – Poison small JSObject derivatives which only contain pointers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

I wrote a script that finds interesting things to poison or generally harden. These stood out because they derive from JSObject and only contain a few pointer or pointer-like fields, and could therefore just be poisoned. This also requires some template "improvements" to our poisoning machinery. Worth noting is that I'm making PoisonedUniquePtr move-assignable and move-constructible from unique_ptr, which makes it a better drop-in replacement because we don't need to use makePoisonedUniquePtr. This means function-locals can be unique_ptr and get the nice RAII pattern, and once the function is done you can just move to the class' PoisonedUniquePtr without worrying.

[Radar WebKit Bug Importer]

<rdar://problem/36407127>

[JF Bastien]

Created attachment 330915 [details]
patch

[EWS Watchlist]

Attachment 330915 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:139:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:142:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 330917 [details]
patch

std:: qualify nullptr_t, and include cstddef as required.

[EWS Watchlist]

Attachment 330917 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Poisoned.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:141:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:144:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 330915 [details]
patch

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

> Source/JavaScriptCore/API/JSAPIWrapperObject.mm:83
> +    , m_wrappedObject(nullptr)

Not needed. Smart pointers initialize to null, unlike raw pointers, including this smart pointer.

[Mark Lam]

Comment on attachment 330917 [details]
patch

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

> Source/JavaScriptCore/API/JSAPIWrapperObject.mm:83
> -    , m_wrappedObject(0)
> +    , m_wrappedObject(nullptr)

You can remove this now.

> Source/JavaScriptCore/runtime/JSArrayBuffer.h:48
> +    ArrayBuffer* impl() const { return m_impl.unpoison(); }

This should be unpoisoned(), not unpoison().

[JF Bastien]

Created attachment 330923 [details]
patch

Address comments.

[EWS Watchlist]

Attachment 330923 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Poisoned.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:141:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:144:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 330932 [details]
patch

MSVC fails the build because the `using JSGlobalObject::PoisonedUniquePtr = ...` re-declaration shadows the new usage in JSCallbackObject. Qualify that new one with WTF:: to disambiguate.

[EWS Watchlist]

Attachment 330932 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Poisoned.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:141:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:144:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Looks like Mac bots can't connect so they're yellow. I ran JSC tests locally and they all pass:
  ./Tools/Scripts/run-javascriptcore-tests --debug --quick --no-build --no-testapi --no-testmasm --no-testb3 --no-testair

[Mark Lam]

Comment on attachment 330932 [details]
patch

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

r=me with feedback addressed.

Please remember to address comments in https://bugs.webkit.org/show_bug.cgi?id=181483#c7.
Please also do a quick benchmark run (sunspider, octane, kraken) to make sure that there are no perf surprises from this patch.  I typically attach the benchmark results from the run for future reference.  I suggest that you do the same.

> Source/JavaScriptCore/ChangeLog:20
> +        I wrote a script that finds interesting things to poison or
> +        generally harden. These stood out because they derive from
> +        JSObject and only contain a few pointer or pointer-like fields,
> +        and could therefore just be poisoned. This also requires some
> +        template "improvements" to our poisoning machinery. Worth noting
> +        is that I'm making PoisonedUniquePtr move-assignable and
> +        move-constructible from unique_ptr, which makes it a better
> +        drop-in replacement because we don't need to use
> +        makePoisonedUniquePtr. This means function-locals can be
> +        unique_ptr and get the nice RAII pattern, and once the function is
> +        done you can just move to the class' PoisonedUniquePtr without
> +        worrying.

Maybe you should consider removing makePoisonedUniquePtr and its uses, and make it so that PoisonedUniquePtr is initialized in a consistent way.  You can do that in a follow up patch.

> Source/WTF/wtf/Poisoned.h:31
> +#include <cstddef>
> +#include <utility>

I think our convention is that these go above the <wtf/...> headers unless there's a reason that they need to go after.  Please fix.

> Source/WTF/wtf/PoisonedUniquePtr.h:32
> +#include <cstddef>
> +#include <memory>

Ditto.

[JF Bastien]

(In reply to Mark Lam from comment #13)
> Comment on attachment 330932 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330932&action=review
> 
> r=me with feedback addressed.
> 
> Please remember to address comments in
> https://bugs.webkit.org/show_bug.cgi?id=181483#c7.

Which? I thought I already did these.

> Please also do a quick benchmark run (sunspider, octane, kraken) to make
> sure that there are no perf surprises from this patch.  I typically attach
> the benchmark results from the run for future reference.  I suggest that you
> do the same.

Will do now.

> > Source/JavaScriptCore/ChangeLog:20
> > +        I wrote a script that finds interesting things to poison or
> > +        generally harden. These stood out because they derive from
> > +        JSObject and only contain a few pointer or pointer-like fields,
> > +        and could therefore just be poisoned. This also requires some
> > +        template "improvements" to our poisoning machinery. Worth noting
> > +        is that I'm making PoisonedUniquePtr move-assignable and
> > +        move-constructible from unique_ptr, which makes it a better
> > +        drop-in replacement because we don't need to use
> > +        makePoisonedUniquePtr. This means function-locals can be
> > +        unique_ptr and get the nice RAII pattern, and once the function is
> > +        done you can just move to the class' PoisonedUniquePtr without
> > +        worrying.
> 
> Maybe you should consider removing makePoisonedUniquePtr and its uses, and
> make it so that PoisonedUniquePtr is initialized in a consistent way.  You
> can do that in a follow up patch.

Agreed, that was my intent but for a follow-up.

> > Source/WTF/wtf/Poisoned.h:31
> > +#include <cstddef>
> > +#include <utility>
> 
> I think our convention is that these go above the <wtf/...> headers unless
> there's a reason that they need to go after.  Please fix.

"Includes of system headers must come after includes of other headers."
  https://webkit.org/code-style-guidelines/#include-system

It's usually a bad idea to include C / STL headers before our own.

[Mark Lam]

(In reply to JF Bastien from comment #14)
> > Please remember to address comments in
> > https://bugs.webkit.org/show_bug.cgi?id=181483#c7.
> 
> Which? I thought I already did these.

Sorry.  I misread my own comment.  I was referring to the style fix comments (which I must not have published at the time though I thought I did).  This brings us to ...

> > > Source/WTF/wtf/Poisoned.h:31
> > > +#include <cstddef>
> > > +#include <utility>
> > 
> > I think our convention is that these go above the <wtf/...> headers unless
> > there's a reason that they need to go after.  Please fix.
> 
> "Includes of system headers must come after includes of other headers."
>   https://webkit.org/code-style-guidelines/#include-system
> 
> It's usually a bad idea to include C / STL headers before our own.

I stand corrected.  Ignore me.

[JF Bastien]

Benchmark results in the noise, with --outer 10 I get "might be 1.0023x faster".

[JF Bastien]

Created attachment 330964 [details]
benchmark results

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 330932 [details]:

The commit-queue is continuing to process your patch.

[EWS Watchlist]

Comment on attachment 330932 [details]
patch

Attachment 330932 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6027709

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[EWS Watchlist]

Created attachment 331007 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[JF Bastien]

Comment on attachment 330932 [details]
patch

Test seems to by flaking by timeout.

[WebKit Commit Bot]

Comment on attachment 330932 [details]
patch

Clearing flags on attachment: 330932

Committed r226752: <https://trac.webkit.org/changeset/226752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to Mark Lam from comment #15)
> > > > Source/WTF/wtf/Poisoned.h:31
> > > > +#include <cstddef>
> > > > +#include <utility>
> > > 
> > > I think our convention is that these go above the <wtf/...> headers unless
> > > there's a reason that they need to go after.  Please fix.
> > 
> > "Includes of system headers must come after includes of other headers."
> >   https://webkit.org/code-style-guidelines/#include-system
> > 
> > It's usually a bad idea to include C / STL headers before our own.
> 
> I stand corrected.  Ignore me.

The intent of that style rule is that <> header includes go after "" header includes.

It’s not intended to be a rule about true “system” headers vs. WebKit headers.

[JF Bastien]

(In reply to Darin Adler from comment #24)
> (In reply to Mark Lam from comment #15)
> > > > > Source/WTF/wtf/Poisoned.h:31
> > > > > +#include <cstddef>
> > > > > +#include <utility>
> > > > 
> > > > I think our convention is that these go above the <wtf/...> headers unless
> > > > there's a reason that they need to go after.  Please fix.
> > > 
> > > "Includes of system headers must come after includes of other headers."
> > >   https://webkit.org/code-style-guidelines/#include-system
> > > 
> > > It's usually a bad idea to include C / STL headers before our own.
> > 
> > I stand corrected.  Ignore me.
> 
> The intent of that style rule is that <> header includes go after "" header
> includes.
> 
> It’s not intended to be a rule about true “system” headers vs. WebKit
> headers.

Huh, looks like I stand corrected ;-)

Why is that? Seems pretty odd TBH. Doesn't that mean that WTF headers see "more" STL and system stuff depending on where they're included from? I guess if we have strict include-what-you-use mentality then WTF headers won't care? I'm also slightly worried about C header's liking of macros, but that might just be a historic MSVC 6 reflex I have (I'm thinking specifically of MSVC headers #define'ing min and max...).

[Darin Adler]

(In reply to JF Bastien from comment #25)
> Why is that? Seems pretty odd TBH.

We just wanted a canonical order for includes. Rather than a variety of different opinions about what is a "logical" order to sort includes. We chose the order that is produced by the "sort" command line tool.

> Doesn't that mean that WTF headers see
> "more" STL and system stuff depending on where they're included from?

Sure. All headers see more or less of other headers depending on where they are included from. That’s not specific to system headers.

> I
> guess if we have strict include-what-you-use mentality then WTF headers
> won't care?

Exactly.

> I'm also slightly worried about C header's liking of macros, but
> that might just be a historic MSVC 6 reflex I have (I'm thinking
> specifically of MSVC headers #define'ing min and max...).

It’s true that Windows headers (not standard library headers, but Windows-specific ones) have historically had some mistakes that led to tricky rules about order of including them. When we find problematic things we can mention those and make exceptions to the normal style rule for them. I remember something about <winsock.h>, but I don’t know of anything that currently affects WebKit.

As far as the Windows thing about macros named "min" and "max", we define NOMIMAX in the makefiles () to sidestep that problem without having to do anything special in the C/C++ code.

[JF Bastien]

(In reply to Darin Adler from comment #26)
> (In reply to JF Bastien from comment #25)
> > Why is that? Seems pretty odd TBH.
> 
> We just wanted a canonical order for includes. Rather than a variety of
> different opinions about what is a "logical" order to sort includes. We
> chose the order that is produced by the "sort" command line tool.

Thanks for clarifying. I'd just misunderstood the guide by assuming it said what I would have chosen. It's a sane choice and has been working for a *bit* of time so it can't be worth arguing over :-)

  https://bugs.webkit.org/show_bug.cgi?id=181521