Patch coming.
<rdar://problem/35916875>
Created attachment 328732 [details] proposed patch.
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.
Created attachment 328733 [details] proposed patch.
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.
Created attachment 328736 [details] x86_64 benchmark results. Perf appears to be neutral.
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).
(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 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.
(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 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.
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
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).
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.
Thanks for the review. Landed in r225659: <http://trac.webkit.org/r225659>.
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).