Bug 180541 - Apply poisoning to some native code and heap pointers.
Summary: Apply poisoning to some native code and heap pointers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-07 13:04 PST by Mark Lam
Modified: 2018-01-02 22:22 PST (History)
12 users (show)

See Also:


Attachments
proposed patch. (28.13 KB, patch)
2017-12-07 13:39 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (29.78 KB, patch)
2017-12-07 13:52 PST, Mark Lam
fpizlo: review+
ews: commit-queue-
Details | Formatted Diff | Diff
x86_64 benchmark results. (8.17 KB, text/plain)
2017-12-07 14:05 PST, Mark Lam
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (408.77 KB, application/zip)
2017-12-07 14:40 PST, Build Bot
no flags Details
patch for landing. (30.78 KB, patch)
2017-12-07 15:48 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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).