Bug 180541 – Apply poisoning to some native code and heap pointers.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/35916875>

[Mark Lam]

Created attachment 328732 [details]
proposed patch.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 328733 [details]
proposed patch.

[EWS Watchlist]

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.

[Mark Lam]

Created attachment 328736 [details]
x86_64 benchmark results.

Perf appears to be neutral.

[Filip Pizlo]

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

[Mark Lam]

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

[Mark Lam]

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.

[Filip Pizlo]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Mark Lam]

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

[EWS Watchlist]

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.

[Mark Lam]

Thanks for the review.  Landed in r225659: <http://trac.webkit.org/r225659>.

[Saam Barati]

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