firefox w^x

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

firefox w^x

Ted Unangst-6
So I was supposed to be working on making the JIT engine conform to W^X a few
months ago. It took a bit longer than expected, but I had a mostly working
patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
Last week I was just starting to feed that patch upstream to firefox, when I
found out about another developer who had already done similar work. Sigh.

The official firefox patch seems likely to ship in some future version, which
is good news for everyone. It's quite similar to the patch I had (though more
polished). To make it available sooner for OpenBSD, here's a backport to the
Firefox in ports.

I haven't been able to test this very much, as I'm still at BSDCan, but when I
get back next week I hope to be able to devote more time to finalizing this
patch. Posting now to let people know it's coming and to give a preview if
you're interested.


--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_irregexp_NativeRegExpMacroAssembler_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,12 @@
+$OpenBSD$
+--- js/src/irregexp/NativeRegExpMacroAssembler.cpp.orig Sat Jun 13 13:54:41 2015
++++ js/src/irregexp/NativeRegExpMacroAssembler.cpp Sat Jun 13 13:55:10 2015
+@@ -450,6 +450,8 @@ NativeRegExpMacroAssembler::GenerateCode(JSContext* cx
+     writePerfSpewerJitCodeProfile(code, "RegExp");
+ #endif
+
++    AutoWritableJitCode awjc(code);
++
+     for (size_t i = 0; i < labelPatches.length(); i++) {
+         LabelPatch& v = labelPatches[i];
+         MOZ_ASSERT(!v.label);
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_BaselineCompiler_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,39 @@
+$OpenBSD$
+--- js/src/jit/BaselineCompiler.cpp.orig Sat Jun 13 13:55:40 2015
++++ js/src/jit/BaselineCompiler.cpp Sat Jun 13 13:57:13 2015
+@@ -226,7 +226,13 @@ BaselineCompiler::compile()
+     // Adopt fallback stubs from the compiler into the baseline script.
+     baselineScript->adoptFallbackStubs(&stubSpace_);
+
+-    // Patch IC loads using IC entries
++    // All barriers are emitted off-by-default, toggle them on if needed.
++    if (cx->zone()->needsIncrementalBarrier())
++        baselineScript->toggleBarriers(true);
++    if (cx->runtime()->jitRuntime()->isProfilerInstrumentationEnabled(cx->runtime()))
++    baselineScript->toggleProfilerInstrumentation(true);
++    AutoWritableJitCode awjc(code);
++
+     for (size_t i = 0; i < icLoadLabels_.length(); i++) {
+         CodeOffsetLabel label = icLoadLabels_[i].label;
+         label.fixup(&masm);
+@@ -240,9 +246,6 @@ BaselineCompiler::compile()
+     if (modifiesArguments_)
+         baselineScript->setModifiesArguments();
+
+-    // All barriers are emitted off-by-default, toggle them on if needed.
+-    if (cx->zone()->needsIncrementalBarrier())
+-        baselineScript->toggleBarriers(true);
+
+ #ifdef JS_TRACE_LOGGING
+     // Initialize the tracelogger instrumentation.
+@@ -260,10 +263,6 @@ BaselineCompiler::compile()
+
+     if (compileDebugInstrumentation_)
+         baselineScript->setHasDebugInstrumentation();
+-
+-    // If profiler instrumentation is enabled, toggle instrumentation on.
+-    if (cx->runtime()->jitRuntime()->isProfilerInstrumentationEnabled(cx->runtime()))
+-        baselineScript->toggleProfilerInstrumentation(true);
+
+     // Always register a native => bytecode mapping entry, since profiler can be
+     // turned on with baseline jitcode on stack, and baseline jitcode cannot be invalidated.
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_BaselineJIT_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,38 @@
+$OpenBSD$
+--- js/src/jit/BaselineJIT.cpp.orig Sat Jun 13 13:57:28 2015
++++ js/src/jit/BaselineJIT.cpp Sat Jun 13 13:59:10 2015
+@@ -841,6 +841,8 @@ BaselineScript::toggleDebugTraps(JSScript* script, jsb
+
+     SrcNoteLineScanner scanner(script->notes(), script->lineno());
+
++    AutoWritableJitCode awjc(method());
++
+     for (uint32_t i = 0; i < numPCMappingIndexEntries(); i++) {
+         PCMappingIndexEntry& entry = pcMappingIndexEntry(i);
+
+@@ -887,6 +889,7 @@ BaselineScript::initTraceLogger(JSRuntime* runtime, JS
+         traceLoggerScriptEvent_ = TraceLoggerEvent(logger, TraceLogger_Scripts);
+
+     if (TraceLogTextIdEnabled(TraceLogger_Engine) || TraceLogTextIdEnabled(TraceLogger_Scripts)) {
++    AutoWritableJitCode awjc(method_);
+         CodeLocationLabel enter(method_, CodeOffsetLabel(traceLoggerEnterToggleOffset_));
+         CodeLocationLabel exit(method_, CodeOffsetLabel(traceLoggerExitToggleOffset_));
+         Assembler::ToggleToCmp(enter);
+@@ -910,6 +913,8 @@ BaselineScript::toggleTraceLoggerScripts(JSRuntime* ru
+     else
+         traceLoggerScriptEvent_ = TraceLoggerEvent(logger, TraceLogger_Scripts);
+
++    AutoWritableJitCode awjc(method());
++
+     // Enable/Disable the traceLogger prologue and epilogue.
+     CodeLocationLabel enter(method_, CodeOffsetLabel(traceLoggerEnterToggleOffset_));
+     CodeLocationLabel exit(method_, CodeOffsetLabel(traceLoggerExitToggleOffset_));
+@@ -963,6 +968,8 @@ BaselineScript::toggleProfilerInstrumentation(bool ena
+
+     JitSpew(JitSpew_BaselineIC, "  toggling profiling %s for BaselineScript %p",
+             enable ? "on" : "off", this);
++
++    AutoWritableJitCode awjc(method());
+
+     // Toggle the jump
+     CodeLocationLabel enterToggleLocation(method_, CodeOffsetLabel(profilerEnterToggleOffset_));
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_CodeGenerator_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,96 @@
+$OpenBSD$
+--- js/src/jit/CodeGenerator.cpp.orig Sat Jun 13 13:59:27 2015
++++ js/src/jit/CodeGenerator.cpp Sat Jun 13 14:02:49 2015
+@@ -7525,11 +7525,45 @@ CodeGenerator::link(JSContext* cx, CompilerConstraintL
+
+     script->setIonScript(cx, ionScript);
+
+-    invalidateEpilogueData_.fixup(&masm);
+-    Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, invalidateEpilogueData_),
+-                                       ImmPtr(ionScript),
+-                                       ImmPtr((void*)-1));
++    {
++        AutoWritableJitCode awjc(code);
++        invalidateEpilogueData_.fixup(&masm);
++        Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, invalidateEpilogueData_),
++                                           ImmPtr(ionScript),
++                                           ImmPtr((void*)-1));
+
++        for (size_t i = 0; i < ionScriptLabels_.length(); i++) {
++            ionScriptLabels_[i].fixup(&masm);
++            Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, ionScriptLabels_[i]),
++                                               ImmPtr(ionScript),
++                                               ImmPtr((void*)-1));
++        }
++
++#ifdef JS_TRACE_LOGGING
++    TraceLoggerThread *logger = TraceLoggerForMainThread(cx->runtime());
++    for (uint32_t i = 0; i < patchableTraceLoggers_.length(); i++) {
++        patchableTraceLoggers_[i].fixup(&masm);
++        Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, patchableTraceLoggers_[i]),
++                                           ImmPtr(logger),
++                                           ImmPtr(nullptr));
++    }
++
++    if (patchableTLScripts_.length() > 0) {
++        MOZ_ASSERT(TraceLogTextIdEnabled(TraceLogger_Scripts));
++        TraceLoggerEvent event(logger, TraceLogger_Scripts, script);
++        ionScript->setTraceLoggerEvent(event);
++        uint32_t textId = event.payload()->textId();
++        for (uint32_t i = 0; i < patchableTLScripts_.length(); i++) {
++            patchableTLScripts_[i].fixup(&masm);
++            Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, patchableTLScripts_[i]),
++                                               ImmPtr((void *) uintptr_t(textId)),
++                                               ImmPtr((void *)0));
++ }
++    }
++#endif
++    }
++
++
+     JitSpew(JitSpew_Codegen, "Created IonScript %p (raw %p)",
+             (void*) ionScript, (void*) code->raw());
+
+@@ -7546,13 +7580,6 @@ CodeGenerator::link(JSContext* cx, CompilerConstraintL
+         perfSpewer_.writeProfile(script, code, masm);
+ #endif
+
+-    for (size_t i = 0; i < ionScriptLabels_.length(); i++) {
+-        ionScriptLabels_[i].fixup(&masm);
+-        Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, ionScriptLabels_[i]),
+-                                           ImmPtr(ionScript),
+-                                           ImmPtr((void*)-1));
+-    }
+-
+     // for generating inline caches during the execution.
+     if (runtimeData_.length())
+         ionScript->copyRuntimeData(&runtimeData_[0]);
+@@ -7580,28 +7607,6 @@ CodeGenerator::link(JSContext* cx, CompilerConstraintL
+     if (patchableBackedges_.length() > 0)
+         ionScript->copyPatchableBackedges(cx, code, patchableBackedges_.begin(), masm);
+
+-#ifdef JS_TRACE_LOGGING
+-    TraceLoggerThread* logger = TraceLoggerForMainThread(cx->runtime());
+-    for (uint32_t i = 0; i < patchableTraceLoggers_.length(); i++) {
+-        patchableTraceLoggers_[i].fixup(&masm);
+-        Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, patchableTraceLoggers_[i]),
+-                                           ImmPtr(logger),
+-                                           ImmPtr(nullptr));
+-    }
+-
+-    if (patchableTLScripts_.length() > 0) {
+-        MOZ_ASSERT(TraceLogTextIdEnabled(TraceLogger_Scripts));
+-        TraceLoggerEvent event(logger, TraceLogger_Scripts, script);
+-        ionScript->setTraceLoggerEvent(event);
+-        uint32_t textId = event.payload()->textId();
+-        for (uint32_t i = 0; i < patchableTLScripts_.length(); i++) {
+-            patchableTLScripts_[i].fixup(&masm);
+-            Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, patchableTLScripts_[i]),
+-                                               ImmPtr((void*) uintptr_t(textId)),
+-                                               ImmPtr((void*)0));
+-        }
+-    }
+-#endif
+
+     // Replace dummy JSObject pointers embedded by LNurseryObject.
+     code->fixupNurseryObjects(cx, gen->nurseryObjects());
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_ExecutableAllocatorPosix_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,22 @@
+$OpenBSD$
+--- js/src/jit/ExecutableAllocatorPosix.cpp.orig Sat Jun 13 14:04:07 2015
++++ js/src/jit/ExecutableAllocatorPosix.cpp Sat Jun 13 14:04:47 2015
+@@ -70,11 +70,9 @@ void ExecutableAllocator::systemRelease(const Executab
+     DeallocateExecutableMemory(alloc.pages, alloc.size, pageSize);
+ }
+
+-#if WTF_ENABLE_ASSEMBLER_WX_EXCLUSIVE
+ void ExecutableAllocator::reprotectRegion(void* start, size_t size, ProtectionSetting setting)
+ {
+-    if (!pageSize)
+-        intializePageSize();
++ MOZ_ASSERT(pageSize);
+
+     // Calculate the start of the page containing this region,
+     // and account for this extra memory within size.
+@@ -89,5 +87,4 @@ void ExecutableAllocator::reprotectRegion(void* start,
+
+     mprotect(pageStart, size, (setting == Writable) ? PROTECTION_FLAGS_RW : PROTECTION_FLAGS_RX);
+ }
+-#endif
+
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_ExecutableAllocator_h Sat Jun 13 14:16:15 2015
@@ -0,0 +1,46 @@
+$OpenBSD$
+--- js/src/jit/ExecutableAllocator.h.orig Sat Jun 13 14:03:05 2015
++++ js/src/jit/ExecutableAllocator.h Sat Jun 13 14:03:51 2015
+@@ -57,13 +57,9 @@ extern  "C" void sync_instruction_memory(caddr_t v, u_
+ #include <sys/cachectl.h>
+ #endif
+
+-#if ENABLE_ASSEMBLER_WX_EXCLUSIVE
+ #define PROTECTION_FLAGS_RW (PROT_READ | PROT_WRITE)
+ #define PROTECTION_FLAGS_RX (PROT_READ | PROT_EXEC)
+ #define INITIAL_PROTECTION_FLAGS PROTECTION_FLAGS_RX
+-#else
+-#define INITIAL_PROTECTION_FLAGS (PROT_READ | PROT_WRITE | PROT_EXEC)
+-#endif
+
+ namespace JS {
+     struct CodeSizes;
+@@ -379,7 +375,6 @@ class ExecutableAllocator {
+         return pool;
+     }
+
+-#if ENABLE_ASSEMBLER_WX_EXCLUSIVE
+     static void makeWritable(void* start, size_t size)
+     {
+         reprotectRegion(start, size, Writable);
+@@ -389,10 +384,6 @@ class ExecutableAllocator {
+     {
+         reprotectRegion(start, size, Executable);
+     }
+-#else
+-    static void makeWritable(void*, size_t) {}
+-    static void makeExecutable(void*, size_t) {}
+-#endif
+
+
+ #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
+@@ -446,9 +437,7 @@ class ExecutableAllocator {
+     ExecutableAllocator(const ExecutableAllocator&) = delete;
+     void operator=(const ExecutableAllocator&) = delete;
+
+-#if ENABLE_ASSEMBLER_WX_EXCLUSIVE
+     static void reprotectRegion(void*, size_t, ProtectionSetting);
+-#endif
+
+     // These are strong references;  they keep pools alive.
+     static const size_t maxSmallPools = 4;
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_IonCaches_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,20 @@
+$OpenBSD$
+--- js/src/jit/IonCaches.cpp.orig Sat Jun 13 14:07:41 2015
++++ js/src/jit/IonCaches.cpp Sat Jun 13 14:08:21 2015
+@@ -246,6 +246,7 @@ class IonCache::StubAttacher
+
+     void patchStubCodePointer(MacroAssembler& masm, JitCode* code) {
+         if (hasStubCodePatchOffset_) {
++ AutoWritableJitCode awjc(code);
+             stubCodePatchOffset_.fixup(&masm);
+             Assembler::PatchDataWithValueCheck(CodeLocationLabel(code, stubCodePatchOffset_),
+                                                ImmPtr(code), STUB_ADDR);
+@@ -312,6 +313,8 @@ RepatchIonCache::bindInitialJump(MacroAssembler& masm,
+ void
+ RepatchIonCache::updateBaseAddress(JitCode* code, MacroAssembler& masm)
+ {
++ AutoWritableJitCode awjc(code);
++
+     IonCache::updateBaseAddress(code, masm);
+     initialJump_.repoint(code, &masm);
+     lastJump_.repoint(code, &masm);
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_IonCode_h Sat Jun 13 14:16:15 2015
@@ -0,0 +1,14 @@
+$OpenBSD$
+--- js/src/jit/IonCode.h.orig Sat Jun 13 14:10:25 2015
++++ js/src/jit/IonCode.h Sat Jun 13 14:10:46 2015
+@@ -107,6 +107,10 @@ class JitCode : public gc::TenuredCell
+     size_t instructionsSize() const {
+         return insnSize_;
+     }
++    size_t bufferSize() const {
++    return bufferSize_;
++
++    }
+     void trace(JSTracer* trc);
+     void finalize(FreeOp* fop);
+     void fixupAfterMovingGC() {}
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_Ion_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,49 @@
+$OpenBSD$
+--- js/src/jit/Ion.cpp.orig Sat Jun 13 14:05:05 2015
++++ js/src/jit/Ion.cpp Sat Jun 13 14:07:32 2015
+@@ -626,6 +626,8 @@ JitCode::trace(JSTracer* trc)
+     if (invalidated())
+         return;
+
++    AutoWritableJitCode awjc(this);
++
+     if (jumpRelocTableBytes_) {
+         uint8_t* start = code_ + jumpRelocTableOffset();
+         CompactBufferReader reader(start, start + jumpRelocTableBytes_);
+@@ -644,6 +646,8 @@ JitCode::fixupNurseryObjects(JSContext* cx, const Obje
+     if (nurseryObjects.empty() || !dataRelocTableBytes_)
+         return;
+
++    AutoWritableJitCode awjc(this);
++
+     uint8_t* start = code_ + dataRelocTableOffset();
+     CompactBufferReader reader(start, start + dataRelocTableBytes_);
+     MacroAssembler::FixupNurseryObjects(cx, this, reader, nurseryObjects);
+@@ -663,7 +667,10 @@ JitCode::finalize(FreeOp* fop)
+     // Buffer can be freed at any time hereafter. Catch use-after-free bugs.
+     // Don't do this if the Ion code is protected, as the signal handler will
+     // deadlock trying to reacquire the interrupt lock.
+-    memset(code_, JS_SWEPT_CODE_PATTERN, bufferSize_);
++    {
++ AutoWritableJitCode awjc(this);
++ memset(code_, JS_SWEPT_CODE_PATTERN, bufferSize_);
++    }
+     code_ = nullptr;
+
+     // Code buffers are stored inside JSC pools.
+@@ -680,6 +687,7 @@ JitCode::finalize(FreeOp* fop)
+ void
+ JitCode::togglePreBarriers(bool enabled)
+ {
++ AutoWritableJitCode awjc(this);
+     uint8_t* start = code_ + preBarrierTableOffset();
+     CompactBufferReader reader(start, start + preBarrierTableBytes_);
+
+@@ -2590,6 +2598,7 @@ InvalidateActivation(FreeOp* fop, const JitActivationI
+         // the call sequence causing the safepoint being >= the size of
+         // a uint32, which is checked during safepoint index
+         // construction.
++ AutoWritableJitCode awjc(ionCode);
+         const SafepointIndex* si = ionScript->getSafepointIndex(it.returnAddressToFp());
+         CodeLocationLabel dataLabelToMunge(it.returnAddressToFp());
+         ptrdiff_t delta = ionScript->invalidateEpilogueDataOffset() -
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_Linker_h Sat Jun 13 14:16:15 2015
@@ -0,0 +1,11 @@
+$OpenBSD$
+--- js/src/jit/Linker.h.orig Sat Jun 13 14:10:54 2015
++++ js/src/jit/Linker.h Sat Jun 13 14:11:25 2015
+@@ -68,6 +68,7 @@ class Linker
+             return nullptr;
+         if (masm.oom())
+             return fail(cx);
++ AutoWritableJitCode awjc(result, bytesNeeded);
+         code->copyFrom(masm);
+         masm.link(code);
+         if (masm.embedsNurseryPointers())
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_jit_x86_Assembler-x86_h Sat Jun 13 14:16:15 2015
@@ -0,0 +1,11 @@
+$OpenBSD$
+--- js/src/jit/x86/Assembler-x86.h.orig Sat Jun 13 14:11:43 2015
++++ js/src/jit/x86/Assembler-x86.h Sat Jun 13 14:11:59 2015
+@@ -164,6 +164,7 @@ PatchJump(CodeLocationJump jump, CodeLocationLabel lab
+     MOZ_ASSERT(((*x >= 0x80 && *x <= 0x8F) && *(x - 1) == 0x0F) ||
+                (*x == 0xE9));
+ #endif
++    AutoWritableJitCode awjc(jump.raw() - 8, 8);
+     X86Encoding::SetRel32(jump.raw(), label.raw());
+ }
+ static inline void
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_vm_Runtime_cpp Sat Jun 13 14:16:15 2015
@@ -0,0 +1,11 @@
+$OpenBSD$
+--- js/src/vm/Runtime.cpp.orig Sat Jun 13 14:12:26 2015
++++ js/src/vm/Runtime.cpp Sat Jun 13 14:12:47 2015
+@@ -206,6 +206,7 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime)
+     ctypesActivityCallback(nullptr),
+     offthreadIonCompilationEnabled_(true),
+     parallelParsingEnabled_(true),
++    autoWritableJitCodeActive_(false),
+ #ifdef DEBUG
+     enteredPolicy(nullptr),
+ #endif
--- /dev/null Sat Jun 13 16:06:25 2015
+++ patches/patch-js_src_vm_Runtime_h Sat Jun 13 14:16:15 2015
@@ -0,0 +1,57 @@
+$OpenBSD$
+--- js/src/vm/Runtime.h.orig Sat Jun 13 14:12:57 2015
++++ js/src/vm/Runtime.h Sat Jun 13 14:15:02 2015
+@@ -1305,6 +1305,8 @@ struct JSRuntime : public JS::shadow::Runtime,
+     bool offthreadIonCompilationEnabled_;
+     bool parallelParsingEnabled_;
+
++    bool autoWritableJitCodeActive_;
++
+   public:
+
+     // Note: these values may be toggled dynamically (in response to about:config
+@@ -1322,6 +1324,11 @@ struct JSRuntime : public JS::shadow::Runtime,
+         return parallelParsingEnabled_;
+     }
+
++    void toggleAutoWritableJitCodeActive(bool b) {
++    MOZ_ASSERT(autoWritableJitCodeActive_ != b);
++    autoWritableJitCodeActive_ = b;
++    }
++
+     const JS::RuntimeOptions& options() const {
+         return options_;
+     }
+@@ -1714,6 +1721,32 @@ class AutoEnterIonCompilation
+
+     MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
+ };
++
++class MOZ_STACK_CLASS AutoWritableJitCode
++{
++    JSRuntime *rt_;
++    void *addr_;
++    size_t size_;
++
++  public:
++    AutoWritableJitCode(JSRuntime *rt, void *addr, size_t size)
++      : rt_(rt), addr_(addr), size_(size)
++    {
++        rt_->toggleAutoWritableJitCodeActive(true);
++        jit::ExecutableAllocator::makeWritable(addr_, size_);
++    }
++    AutoWritableJitCode(void *addr, size_t size)
++      : AutoWritableJitCode(TlsPerThreadData.get()->runtimeFromMainThread(), addr, size)
++    {}
++    explicit AutoWritableJitCode(jit::JitCode *code)
++      : AutoWritableJitCode(code->runtimeFromMainThread(), code->raw(), code->bufferSize())
++    {}
++    ~AutoWritableJitCode() {
++        jit::ExecutableAllocator::makeExecutable(addr_, size_);
++        rt_->toggleAutoWritableJitCodeActive(false);
++    }
++};
++
+
+ } /* namespace js */
+

Reply | Threaded
Open this post in threaded view
|

firefox w^x again

David Coppa
On Sat, 13 Jun 2015, Ted Unangst wrote:

> So I was supposed to be working on making the JIT engine conform to W^X a few
> months ago. It took a bit longer than expected, but I had a mostly working
> patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
> Last week I was just starting to feed that patch upstream to firefox, when I
> found out about another developer who had already done similar work. Sigh.
>
> The official firefox patch seems likely to ship in some future version, which
> is good news for everyone. It's quite similar to the patch I had (though more
> polished). To make it available sooner for OpenBSD, here's a backport to the
> Firefox in ports.
>
> I haven't been able to test this very much, as I'm still at BSDCan, but when I
> get back next week I hope to be able to devote more time to finalizing this
> patch. Posting now to let people know it's coming and to give a preview if
> you're interested.

Hi!

The official patch is now in.

I don't know why it's enabled exclusively for iOS (maybe because it's
the only platform that has been thoroughly tested? Or because major
linux distros are not yet ready), probably Landry can shed some light
on this...

Btw, my firefox-41.0.1 (rebuilt with the patch below) still hasn't
exploded during my tests: routinary browsing, GMail, Google Maps,
Twitter, etc...

Comments? Opinions?

Ciao,
David

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/mozilla-firefox/Makefile,v
retrieving revision 1.280
diff -u -p -u -p -r1.280 Makefile
--- Makefile 5 Oct 2015 08:48:14 -0000 1.280
+++ Makefile 16 Oct 2015 07:52:32 -0000
@@ -8,6 +8,7 @@ MOZILLA_VERSION = 41.0.1
 MOZILLA_BRANCH = release
 MOZILLA_PROJECT = firefox
 MOZILLA_CODENAME = browser
+REVISION = 0
 BROKEN-sparc64 = xpcshell SIGBUS during fake
 EXTRACT_SUFX = .tar.xz
 
Index: patches/patch-js_src_jit_ExecutableAllocator_cpp
===================================================================
RCS file: patches/patch-js_src_jit_ExecutableAllocator_cpp
diff -N patches/patch-js_src_jit_ExecutableAllocator_cpp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-js_src_jit_ExecutableAllocator_cpp 16 Oct 2015 07:52:32 -0000
@@ -0,0 +1,15 @@
+$OpenBSD$
+
+Mark JIT pages as non-writable
+
+--- js/src/jit/ExecutableAllocator.cpp.orig Thu Oct 15 12:47:51 2015
++++ js/src/jit/ExecutableAllocator.cpp Thu Oct 15 12:48:15 2015
+@@ -88,8 +88,4 @@ ExecutableAllocator::addSizeOfCode(JS::CodeSizes* size
+     }
+ }
+
+-#if TARGET_OS_IPHONE
+ bool ExecutableAllocator::nonWritableJitCode = true;
+-#else
+-bool ExecutableAllocator::nonWritableJitCode = false;
+-#endif

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

Landry Breuil-5
On Fri, Oct 16, 2015 at 10:43:53AM +0200, David Coppa wrote:

> On Sat, 13 Jun 2015, Ted Unangst wrote:
>
> > So I was supposed to be working on making the JIT engine conform to W^X a few
> > months ago. It took a bit longer than expected, but I had a mostly working
> > patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
> > Last week I was just starting to feed that patch upstream to firefox, when I
> > found out about another developer who had already done similar work. Sigh.
> >
> > The official firefox patch seems likely to ship in some future version, which
> > is good news for everyone. It's quite similar to the patch I had (though more
> > polished). To make it available sooner for OpenBSD, here's a backport to the
> > Firefox in ports.
> >
> > I haven't been able to test this very much, as I'm still at BSDCan, but when I
> > get back next week I hope to be able to devote more time to finalizing this
> > patch. Posting now to let people know it's coming and to give a preview if
> > you're interested.
>
> Hi!
>
> The official patch is now in.
>
> I don't know why it's enabled exclusively for iOS (maybe because it's
> the only platform that has been thoroughly tested? Or because major
> linux distros are not yet ready), probably Landry can shed some light
> on this...
>
> Btw, my firefox-41.0.1 (rebuilt with the patch below) still hasn't
> exploded during my tests: routinary browsing, GMail, Google Maps,
> Twitter, etc...
>
> Comments? Opinions?

Bring this upstream first :)

Landry

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

David Coppa
On Fri, Oct 16, 2015 at 11:57 AM, Landry Breuil <[hidden email]> wrote:

> On Fri, Oct 16, 2015 at 10:43:53AM +0200, David Coppa wrote:
>> On Sat, 13 Jun 2015, Ted Unangst wrote:
>>
>> > So I was supposed to be working on making the JIT engine conform to W^X a few
>> > months ago. It took a bit longer than expected, but I had a mostly working
>> > patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
>> > Last week I was just starting to feed that patch upstream to firefox, when I
>> > found out about another developer who had already done similar work. Sigh.
>> >
>> > The official firefox patch seems likely to ship in some future version, which
>> > is good news for everyone. It's quite similar to the patch I had (though more
>> > polished). To make it available sooner for OpenBSD, here's a backport to the
>> > Firefox in ports.
>> >
>> > I haven't been able to test this very much, as I'm still at BSDCan, but when I
>> > get back next week I hope to be able to devote more time to finalizing this
>> > patch. Posting now to let people know it's coming and to give a preview if
>> > you're interested.
>>
>> Hi!
>>
>> The official patch is now in.
>>
>> I don't know why it's enabled exclusively for iOS (maybe because it's
>> the only platform that has been thoroughly tested? Or because major
>> linux distros are not yet ready), probably Landry can shed some light
>> on this...
>>
>> Btw, my firefox-41.0.1 (rebuilt with the patch below) still hasn't
>> exploded during my tests: routinary browsing, GMail, Google Maps,
>> Twitter, etc...
>>
>> Comments? Opinions?
>
> Bring this upstream first :)

How? Can you intercede for me?

Cheers!
--
"If you try a few times and give up, you'll never get there. But if
you keep at it... There's a lot of problems in the world which can
really be solved by applying two or three times the persistence that
other people will."
                -- Stewart Nelson

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

Landry Breuil-5
In reply to this post by Landry Breuil-5
On Fri, Oct 16, 2015 at 11:57:40AM +0200, Landry Breuil wrote:

> On Fri, Oct 16, 2015 at 10:43:53AM +0200, David Coppa wrote:
> > On Sat, 13 Jun 2015, Ted Unangst wrote:
> >
> > > So I was supposed to be working on making the JIT engine conform to W^X a few
> > > months ago. It took a bit longer than expected, but I had a mostly working
> > > patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
> > > Last week I was just starting to feed that patch upstream to firefox, when I
> > > found out about another developer who had already done similar work. Sigh.
> > >
> > > The official firefox patch seems likely to ship in some future version, which
> > > is good news for everyone. It's quite similar to the patch I had (though more
> > > polished). To make it available sooner for OpenBSD, here's a backport to the
> > > Firefox in ports.
> > >
> > > I haven't been able to test this very much, as I'm still at BSDCan, but when I
> > > get back next week I hope to be able to devote more time to finalizing this
> > > patch. Posting now to let people know it's coming and to give a preview if
> > > you're interested.
> >
> > Hi!
> >
> > The official patch is now in.
> >
> > I don't know why it's enabled exclusively for iOS (maybe because it's
> > the only platform that has been thoroughly tested? Or because major
> > linux distros are not yet ready), probably Landry can shed some light
> > on this...
> >
> > Btw, my firefox-41.0.1 (rebuilt with the patch below) still hasn't
> > exploded during my tests: routinary browsing, GMail, Google Maps,
> > Twitter, etc...
> >
> > Comments? Opinions?
>
> Bring this upstream first :)

And for the rationale of having it only on iOS, see
https://bugzilla.mozilla.org/show_bug.cgi?id=977805. From quickly
skimming through comments there, apparently it conflicts with asm.js.
Hence, discuss it upstream.

Landry

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

Landry Breuil-5
In reply to this post by David Coppa
On Fri, Oct 16, 2015 at 12:02:07PM +0200, David Coppa wrote:

> On Fri, Oct 16, 2015 at 11:57 AM, Landry Breuil <[hidden email]> wrote:
> > On Fri, Oct 16, 2015 at 10:43:53AM +0200, David Coppa wrote:
> >> On Sat, 13 Jun 2015, Ted Unangst wrote:
> >>
> >> > So I was supposed to be working on making the JIT engine conform to W^X a few
> >> > months ago. It took a bit longer than expected, but I had a mostly working
> >> > patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
> >> > Last week I was just starting to feed that patch upstream to firefox, when I
> >> > found out about another developer who had already done similar work. Sigh.
> >> >
> >> > The official firefox patch seems likely to ship in some future version, which
> >> > is good news for everyone. It's quite similar to the patch I had (though more
> >> > polished). To make it available sooner for OpenBSD, here's a backport to the
> >> > Firefox in ports.
> >> >
> >> > I haven't been able to test this very much, as I'm still at BSDCan, but when I
> >> > get back next week I hope to be able to devote more time to finalizing this
> >> > patch. Posting now to let people know it's coming and to give a preview if
> >> > you're interested.
> >>
> >> Hi!
> >>
> >> The official patch is now in.
> >>
> >> I don't know why it's enabled exclusively for iOS (maybe because it's
> >> the only platform that has been thoroughly tested? Or because major
> >> linux distros are not yet ready), probably Landry can shed some light
> >> on this...
> >>
> >> Btw, my firefox-41.0.1 (rebuilt with the patch below) still hasn't
> >> exploded during my tests: routinary browsing, GMail, Google Maps,
> >> Twitter, etc...
> >>
> >> Comments? Opinions?
> >
> > Bring this upstream first :)
>
> How? Can you intercede for me?

File a bug upstream in 'javascript engine: JIT' component, blocking 977805
and cc the people who worked on it (:jandem, :cpeterson, :nbp,
:ted.mielczarek and :bsmedberg at least)
Sorry, no time for this and no reliable internet @home.

Landry

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

David Coppa
In reply to this post by Landry Breuil-5
On Fri, Oct 16, 2015 at 12:03 PM, Landry Breuil <[hidden email]> wrote:

> On Fri, Oct 16, 2015 at 11:57:40AM +0200, Landry Breuil wrote:
>> On Fri, Oct 16, 2015 at 10:43:53AM +0200, David Coppa wrote:
>> > On Sat, 13 Jun 2015, Ted Unangst wrote:
>> >
>> > > So I was supposed to be working on making the JIT engine conform to W^X a few
>> > > months ago. It took a bit longer than expected, but I had a mostly working
>> > > patch. Then I disappeared from OpenBSD for a bit and took my patch with me.
>> > > Last week I was just starting to feed that patch upstream to firefox, when I
>> > > found out about another developer who had already done similar work. Sigh.
>> > >
>> > > The official firefox patch seems likely to ship in some future version, which
>> > > is good news for everyone. It's quite similar to the patch I had (though more
>> > > polished). To make it available sooner for OpenBSD, here's a backport to the
>> > > Firefox in ports.
>> > >
>> > > I haven't been able to test this very much, as I'm still at BSDCan, but when I
>> > > get back next week I hope to be able to devote more time to finalizing this
>> > > patch. Posting now to let people know it's coming and to give a preview if
>> > > you're interested.
>> >
>> > Hi!
>> >
>> > The official patch is now in.
>> >
>> > I don't know why it's enabled exclusively for iOS (maybe because it's
>> > the only platform that has been thoroughly tested? Or because major
>> > linux distros are not yet ready), probably Landry can shed some light
>> > on this...
>> >
>> > Btw, my firefox-41.0.1 (rebuilt with the patch below) still hasn't
>> > exploded during my tests: routinary browsing, GMail, Google Maps,
>> > Twitter, etc...
>> >
>> > Comments? Opinions?
>>
>> Bring this upstream first :)
>
> And for the rationale of having it only on iOS, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=977805. From quickly
> skimming through comments there, apparently it conflicts with asm.js.
> Hence, discuss it upstream.
>
> Landry

So, the only downside seems to be performance related.

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1215479

<<iOS requires it (because it doesn't allow allocating RWX memory), but it's not
enabled elsewhere because it's a performance hit (we should remeasure how much
though). This is because we have to toggle pages from executable to writable
and back whenever we patch JIT code>>

What about enabling it in our port and see how it will end up?
We can always re-disable it at any time if people complain...

Ciao!
David
--
"If you try a few times and give up, you'll never get there. But if
you keep at it... There's a lot of problems in the world which can
really be solved by applying two or three times the persistence that
other people will."
                -- Stewart Nelson

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

Theo de Raadt
> So, the only downside seems to be performance related.
>
> See: https://bugzilla.mozilla.org/show_bug.cgi?id=1215479
>
> <<iOS requires it (because it doesn't allow allocating RWX memory), but it's not
> enabled elsewhere because it's a performance hit (we should remeasure how much
> though). This is because we have to toggle pages from executable to writable
> and back whenever we patch JIT code>>
>
> What about enabling it in our port and see how it will end up?
> We can always re-disable it at any time if people complain...

We could block OpenBSD from allocating RWX memory tomorrow.  The
blocker against us doing this, is a very small set of programs like
firefox.

This circular reference is really bullshit.

We need to absolutely move forward, get this tested, give them
feedback.  If we can accept the cost etc, and provide feedback
upstream, which results in those 'performance' issues being resolved,
then maybe everyone can run safer software later down the road.

We (as a userbase) are exactly the type of people to help them with this.


Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

Stuart Henderson-6
In reply to this post by David Coppa
On 2015/10/16 14:28, David Coppa wrote:

> So, the only downside seems to be performance related.
>
> See: https://bugzilla.mozilla.org/show_bug.cgi?id=1215479
>
> <<iOS requires it (because it doesn't allow allocating RWX memory), but it's not
> enabled elsewhere because it's a performance hit (we should remeasure how much
> though). This is because we have to toggle pages from executable to writable
> and back whenever we patch JIT code>>
>
> What about enabling it in our port and see how it will end up?
> We can always re-disable it at any time if people complain...

If it's not too much of a performance hit for iOS devices (i.e. small
machines with arm cpu), it should be fine on a standard workstation-type
machine like people would normally use to run Firefox on OpenBSD.

And people have a relatively easy fallback of firefox-esr if they
run into runtime problems.

+1 from me for enabling in our port.

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

David Coppa
In reply to this post by David Coppa
On Fri, 16 Oct 2015, Landry Breuil wrote:

> On Fri, Oct 16, 2015 at 02:28:45PM +0200, David Coppa wrote:
> >
> > So, the only downside seems to be performance related.
> >
> > See: https://bugzilla.mozilla.org/show_bug.cgi?id=1215479
> >
> > <<iOS requires it (because it doesn't allow allocating RWX memory), but it's not
> > enabled elsewhere because it's a performance hit (we should remeasure how much
> > though). This is because we have to toggle pages from executable to writable
> > and back whenever we patch JIT code>>
> >
> > What about enabling it in our port and see how it will end up?
> > We can always re-disable it at any time if people complain...
>
> Commit it but please make the diff eventually upstreamable by adding
> openbsd to the ios case, not removing the ifdefs... and link to the
> bug#.

Here's the diff I'd like to commit:

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/mozilla-firefox/Makefile,v
retrieving revision 1.280
diff -u -p -u -p -r1.280 Makefile
--- Makefile 5 Oct 2015 08:48:14 -0000 1.280
+++ Makefile 16 Oct 2015 15:45:16 -0000
@@ -8,6 +8,7 @@ MOZILLA_VERSION = 41.0.1
 MOZILLA_BRANCH = release
 MOZILLA_PROJECT = firefox
 MOZILLA_CODENAME = browser
+REVISION = 0
 BROKEN-sparc64 = xpcshell SIGBUS during fake
 EXTRACT_SUFX = .tar.xz
 
Index: patches/patch-js_src_jit_ExecutableAllocator_cpp
===================================================================
RCS file: patches/patch-js_src_jit_ExecutableAllocator_cpp
diff -N patches/patch-js_src_jit_ExecutableAllocator_cpp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-js_src_jit_ExecutableAllocator_cpp 16 Oct 2015 15:45:16 -0000
@@ -0,0 +1,17 @@
+$OpenBSD$
+
+Mark JIT pages as non-writable
+
+https://bugzilla.mozilla.org/show_bug.cgi?id=1215479
+
+--- js/src/jit/ExecutableAllocator.cpp.orig Tue Sep 29 23:44:56 2015
++++ js/src/jit/ExecutableAllocator.cpp Fri Oct 16 17:02:15 2015
+@@ -88,7 +88,7 @@ ExecutableAllocator::addSizeOfCode(JS::CodeSizes* size
+     }
+ }
+
+-#if TARGET_OS_IPHONE
++#if defined(__OpenBSD__) || TARGET_OS_IPHONE
+ bool ExecutableAllocator::nonWritableJitCode = true;
+ #else
+ bool ExecutableAllocator::nonWritableJitCode = false;

Reply | Threaded
Open this post in threaded view
|

Re: firefox w^x again

David Coppa
On Fri, 16 Oct 2015, David Coppa wrote:

> On Fri, 16 Oct 2015, Landry Breuil wrote:
>
> > On Fri, Oct 16, 2015 at 02:28:45PM +0200, David Coppa wrote:
> > >
> > > So, the only downside seems to be performance related.
> > >
> > > See: https://bugzilla.mozilla.org/show_bug.cgi?id=1215479
> > >
> > > <<iOS requires it (because it doesn't allow allocating RWX memory), but it's not
> > > enabled elsewhere because it's a performance hit (we should remeasure how much
> > > though). This is because we have to toggle pages from executable to writable
> > > and back whenever we patch JIT code>>
> > >
> > > What about enabling it in our port and see how it will end up?
> > > We can always re-disable it at any time if people complain...
> >
> > Commit it but please make the diff eventually upstreamable by adding
> > openbsd to the ios case, not removing the ifdefs... and link to the
> > bug#.
>
> Here's the diff I'd like to commit:

Ops.

Updated diff against firefox-41.0.2:

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/mozilla-firefox/Makefile,v
retrieving revision 1.281
diff -u -p -u -p -r1.281 Makefile
--- Makefile 16 Oct 2015 12:04:40 -0000 1.281
+++ Makefile 16 Oct 2015 16:16:52 -0000
@@ -8,6 +8,7 @@ MOZILLA_VERSION = 41.0.2
 MOZILLA_BRANCH = release
 MOZILLA_PROJECT = firefox
 MOZILLA_CODENAME = browser
+REVISION = 0
 BROKEN-sparc64 = xpcshell SIGBUS during fake
 EXTRACT_SUFX = .tar.xz
 
Index: patches/patch-js_src_jit_ExecutableAllocator_cpp
===================================================================
RCS file: patches/patch-js_src_jit_ExecutableAllocator_cpp
diff -N patches/patch-js_src_jit_ExecutableAllocator_cpp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-js_src_jit_ExecutableAllocator_cpp 16 Oct 2015 16:16:52 -0000
@@ -0,0 +1,17 @@
+$OpenBSD$
+
+Mark JIT pages as non-writable
+
+https://bugzilla.mozilla.org/show_bug.cgi?id=1215479
+
+--- js/src/jit/ExecutableAllocator.cpp.orig Tue Sep 29 23:44:56 2015
++++ js/src/jit/ExecutableAllocator.cpp Fri Oct 16 17:02:15 2015
+@@ -88,7 +88,7 @@ ExecutableAllocator::addSizeOfCode(JS::CodeSizes* size
+     }
+ }
+
+-#if TARGET_OS_IPHONE
++#if defined(__OpenBSD__) || TARGET_OS_IPHONE
+ bool ExecutableAllocator::nonWritableJitCode = true;
+ #else
+ bool ExecutableAllocator::nonWritableJitCode = false;