Bug 180541

Summary: Apply poisoning to some native code and heap pointers.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, dbates, ews-watchlist, fpizlo, jfbastien, keith_miller, msaboff, rmorisset, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
none
proposed patch.
fpizlo: review+, ews-watchlist: commit-queue-
x86_64 benchmark results.
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
patch for landing. none

Description Mark Lam 2017-12-07 13:04:05 PST
Patch coming.
Comment 1 Radar WebKit Bug Importer 2017-12-07 13:05:11 PST
<rdar://problem/35916875>
Comment 2 Mark Lam 2017-12-07 13:39:19 PST
Created attachment 328732 [details]
proposed patch.
Comment 3 EWS Watchlist 2017-12-07 13:41:16 PST
Attachment 328732 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_globalDataPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_jitCodePoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:33:  g_nativeCodePoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mark Lam 2017-12-07 13:52:38 PST
Created attachment 328733 [details]
proposed patch.
Comment 5 EWS Watchlist 2017-12-07 13:54:55 PST
Attachment 328733 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_globalDataPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_jitCodePoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:33:  g_nativeCodePoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Lam 2017-12-07 14:05:43 PST
Created attachment 328736 [details]
x86_64 benchmark results.

Perf appears to be neutral.
Comment 7 Filip Pizlo 2017-12-07 14:06:09 PST
Comment on attachment 328733 [details]
proposed patch.

Is Poisoned the one that xor's with a value that is random at runtime or the one that adds a fixed offset?

Pointers to global things should be scrambled (xor with random number chosen at runtime).  Pointers to objects in the heap should be poisoned (add a fixed, compile-time-constant offset that has high bits, like 1<<32).
Comment 8 Mark Lam 2017-12-07 14:10:35 PST
(In reply to Filip Pizlo from comment #7)
> Comment on attachment 328733 [details]
> proposed patch.
> 
> Is Poisoned the one that xor's with a value that is random at runtime or the
> one that adds a fixed offset?
> 
> Pointers to global things should be scrambled (xor with random number chosen
> at runtime).  Pointers to objects in the heap should be poisoned (add a
> fixed, compile-time-constant offset that has high bits, like 1<<32).

You may have missed it but I renamed ScrambledPtr to PoisonedImpl, and made it such that it can work with 2 types of poison values:
1. Poisoned specializes PoisonedImpl to takes a random uintptr_t poison value.
2. Int32Poisoned specializes PoisonedImpl to take a constexpr int32_t poison value.  See StructureTransitionTable.h in this patch for examples of this in use.
Comment 9 Mark Lam 2017-12-07 14:18:55 PST
Comment on attachment 328733 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:3
> +        Apply poisoning to some native code pointers.

I should update the title to say "Apply poisoning to some native code and heap pointers." because this patch actually applies to some heap pointers as well.
Comment 10 Filip Pizlo 2017-12-07 14:20:20 PST
(In reply to Mark Lam from comment #8)
> (In reply to Filip Pizlo from comment #7)
> > Comment on attachment 328733 [details]
> > proposed patch.
> > 
> > Is Poisoned the one that xor's with a value that is random at runtime or the
> > one that adds a fixed offset?
> > 
> > Pointers to global things should be scrambled (xor with random number chosen
> > at runtime).  Pointers to objects in the heap should be poisoned (add a
> > fixed, compile-time-constant offset that has high bits, like 1<<32).
> 
> You may have missed it but I renamed ScrambledPtr to PoisonedImpl, and made
> it such that it can work with 2 types of poison values:
> 1. Poisoned specializes PoisonedImpl to takes a random uintptr_t poison
> value.
> 2. Int32Poisoned specializes PoisonedImpl to take a constexpr int32_t poison
> value.  See StructureTransitionTable.h in this patch for examples of this in
> use.

Yup, I missed that.  Thanks for explaining!
Comment 11 EWS Watchlist 2017-12-07 14:40:36 PST
Comment on attachment 328733 [details]
proposed patch.

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

Number of test failures exceeded the failure limit.
Comment 12 EWS Watchlist 2017-12-07 14:40:38 PST
Created attachment 328742 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Mark Lam 2017-12-07 15:48:01 PST
Created attachment 328750 [details]
patch for landing.

This patch also removes 2 invalid assertions in PoisonedImpl's constructors that were causing the layout test failures.  The WinCairo failure is due to the EWS bot running with a stale build (it's trying to link against a symbol that has been renamed).
Comment 14 EWS Watchlist 2017-12-07 15:50:30 PST
Attachment 328750 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:31:  g_globalDataPoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:32:  g_jitCodePoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:33:  g_nativeCodePoison is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Mark Lam 2017-12-07 17:22:53 PST
Thanks for the review.  Landed in r225659: <http://trac.webkit.org/r225659>.
Comment 16 Saam Barati 2018-01-02 22:22:33 PST
Comment on attachment 328750 [details]
patch for landing.

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

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:313
> +    jit.move(JSInterfaceJIT::TrustedImm64(g_nativeCodePoison), X86Registers::esi);

Super late on commenting here. Now that I'm reading the code, I have one suggestion to abstract this code a bit better:
Instead of having to use g_nativeCodePoison in various places w.r.t InternalFunction, I think a better abstraction would be to have a static function like:
InternalFunction::poison() { return g_nativeCodePoison }

And then everywhere you use g_nativeCodePoison w.r.t InternalFunction, you can just call InternalFunction::poison(). This would allow us to easily add a new poison value just for InternalFunction without having to change all Masm code too.

(The same suggestion applies to ClassInfo, NativeExecutable, etc).