Bug 1434230 part 2 - Add masm methods for string access, more Spectre mitigations. r=luke
☠☠ backed out by a20dc29e502d ☠ ☠
authorJan de Mooij <jdemooij@mozilla.com>
Thu, 08 Feb 2018 22:03:47 +0100
changeset 752948 6598194588d7cbdc2553955f92ea357f15320468
parent 752947 9c9ba4938b080344149ec4cd6f8cd564f7af3be3
child 752949 7f67769bbbd8c696518e92a2752a8a5861f408d5
push id98429
push usermak77@bonardo.net
push dateFri, 09 Feb 2018 10:14:12 +0000
reviewersluke
bugs1434230
milestone60.0a1
Bug 1434230 part 2 - Add masm methods for string access, more Spectre mitigations. r=luke
js/src/jit/CodeGenerator.cpp
js/src/jit/MacroAssembler.cpp
js/src/jit/MacroAssembler.h
js/src/vm/String.h
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -1575,17 +1575,17 @@ CreateDependentString::generate(MacroAss
         masm.push(base);
 
         // Adjust the start index address for the above pushes.
         MOZ_ASSERT(startIndexAddress.base == masm.getStackPointer());
         BaseIndex newStartIndexAddress = startIndexAddress;
         newStartIndexAddress.offset += 2 * sizeof(void*);
 
         // Load chars pointer for the new string.
-        masm.addPtr(ImmWord(JSInlineString::offsetOfInlineStorage()), string);
+        masm.loadInlineStringCharsForStore(string, string);
 
         // Load the source characters pointer.
         masm.loadStringChars(base, temp2);
         masm.load32(newStartIndexAddress, base);
         if (latin1)
             masm.addPtr(temp2, base);
         else
             masm.computeEffectiveAddress(BaseIndex(temp2, base, TimesTwo), base);
@@ -1609,34 +1609,34 @@ CreateDependentString::generate(MacroAss
         // Make a dependent string.
         int32_t flags = (latin1 ? JSString::LATIN1_CHARS_BIT : 0) | JSString::DEPENDENT_FLAGS;
 
         masm.newGCString(string, temp2, &fallbacks_[FallbackKind::NotInlineString]);
         masm.bind(&joins_[FallbackKind::NotInlineString]);
         masm.store32(Imm32(flags), Address(string, JSString::offsetOfFlags()));
         masm.store32(temp1, Address(string, JSString::offsetOfLength()));
 
-        masm.loadPtr(Address(base, JSString::offsetOfNonInlineChars()), temp1);
+        masm.loadNonInlineStringChars(base, temp1);
         masm.load32(startIndexAddress, temp2);
         if (latin1)
             masm.addPtr(temp2, temp1);
         else
             masm.computeEffectiveAddress(BaseIndex(temp1, temp2, TimesTwo), temp1);
-        masm.storePtr(temp1, Address(string, JSString::offsetOfNonInlineChars()));
-        masm.storePtr(base, Address(string, JSDependentString::offsetOfBase()));
+        masm.storeNonInlineStringChars(temp1, string);
+        masm.storeDependentStringBase(base, string);
 
         // Follow any base pointer if the input is itself a dependent string.
         // Watch for undepended strings, which have a base pointer but don't
         // actually share their characters with it.
         Label noBase;
         masm.load32(Address(base, JSString::offsetOfFlags()), temp1);
         masm.and32(Imm32(JSString::TYPE_FLAGS_MASK), temp1);
         masm.branch32(Assembler::NotEqual, temp1, Imm32(JSString::DEPENDENT_FLAGS), &noBase);
-        masm.loadPtr(Address(base, JSDependentString::offsetOfBase()), temp1);
-        masm.storePtr(temp1, Address(string, JSDependentString::offsetOfBase()));
+        masm.loadDependentStringBase(base, temp1);
+        masm.storeDependentStringBase(temp1, string);
         masm.bind(&noBase);
     }
 
     masm.bind(&done);
 }
 
 static void*
 AllocateString(JSContext* cx)
@@ -7824,17 +7824,17 @@ ConcatInlineString(MacroAssembler& masm,
         masm.store32(Imm32(flags), Address(output, JSString::offsetOfFlags()));
     }
     masm.bind(&allocDone);
 
     // Store length.
     masm.store32(temp2, Address(output, JSString::offsetOfLength()));
 
     // Load chars pointer in temp2.
-    masm.computeEffectiveAddress(Address(output, JSInlineString::offsetOfInlineStorage()), temp2);
+    masm.loadInlineStringCharsForStore(output, temp2);
 
     {
         // Copy lhs chars. Note that this advances temp2 to point to the next
         // char. This also clobbers the lhs register.
         if (isTwoByte) {
             CopyStringCharsMaybeInflate(masm, lhs, temp2, temp1, temp3);
         } else {
             masm.loadStringLength(lhs, temp3);
@@ -7905,77 +7905,75 @@ CodeGenerator::visitSubstr(LSubstr* lir)
     // Use slow path for ropes.
     masm.bind(&nonZero);
     masm.branchIfRopeOrExternal(string, temp, slowPath);
 
     // Handle inlined strings by creating a FatInlineString.
     masm.branchTest32(Assembler::Zero, stringFlags, Imm32(JSString::INLINE_CHARS_BIT), &notInline);
     masm.newGCFatInlineString(output, temp, slowPath);
     masm.store32(length, Address(output, JSString::offsetOfLength()));
-    Address stringStorage(string, JSInlineString::offsetOfInlineStorage());
-    Address outputStorage(output, JSInlineString::offsetOfInlineStorage());
 
     masm.branchLatin1String(string, &isInlinedLatin1);
     {
         masm.store32(Imm32(JSString::INIT_FAT_INLINE_FLAGS),
                      Address(output, JSString::offsetOfFlags()));
-        masm.computeEffectiveAddress(stringStorage, temp);
+        masm.loadInlineStringChars(string, temp);
         if (temp2 == string)
             masm.push(string);
         BaseIndex chars(temp, begin, ScaleFromElemWidth(sizeof(char16_t)));
         masm.computeEffectiveAddress(chars, temp2);
-        masm.computeEffectiveAddress(outputStorage, temp);
+        masm.loadInlineStringCharsForStore(output, temp);
         CopyStringChars(masm, temp, temp2, length, temp3, sizeof(char16_t), sizeof(char16_t));
         masm.load32(Address(output, JSString::offsetOfLength()), length);
         masm.store16(Imm32(0), Address(temp, 0));
         if (temp2 == string)
             masm.pop(string);
         masm.jump(done);
     }
     masm.bind(&isInlinedLatin1);
     {
         masm.store32(Imm32(JSString::INIT_FAT_INLINE_FLAGS | JSString::LATIN1_CHARS_BIT),
                      Address(output, JSString::offsetOfFlags()));
         if (temp2 == string)
             masm.push(string);
-        masm.computeEffectiveAddress(stringStorage, temp2);
+        masm.loadInlineStringChars(string, temp2);
         static_assert(sizeof(char) == 1, "begin index shouldn't need scaling");
         masm.addPtr(begin, temp2);
-        masm.computeEffectiveAddress(outputStorage, temp);
+        masm.loadInlineStringCharsForStore(output, temp);
         CopyStringChars(masm, temp, temp2, length, temp3, sizeof(char), sizeof(char));
         masm.load32(Address(output, JSString::offsetOfLength()), length);
         masm.store8(Imm32(0), Address(temp, 0));
         if (temp2 == string)
             masm.pop(string);
         masm.jump(done);
     }
 
     // Handle other cases with a DependentString.
     masm.bind(&notInline);
     masm.newGCString(output, temp, slowPath);
     masm.store32(length, Address(output, JSString::offsetOfLength()));
-    masm.storePtr(string, Address(output, JSDependentString::offsetOfBase()));
+    masm.storeDependentStringBase(string, output);
 
     masm.branchLatin1String(string, &isLatin1);
     {
         masm.store32(Imm32(JSString::DEPENDENT_FLAGS), Address(output, JSString::offsetOfFlags()));
-        masm.loadPtr(Address(string, JSString::offsetOfNonInlineChars()), temp);
+        masm.loadNonInlineStringChars(string, temp);
         BaseIndex chars(temp, begin, ScaleFromElemWidth(sizeof(char16_t)));
         masm.computeEffectiveAddress(chars, temp);
-        masm.storePtr(temp, Address(output, JSString::offsetOfNonInlineChars()));
+        masm.storeNonInlineStringChars(temp, output);
         masm.jump(done);
     }
     masm.bind(&isLatin1);
     {
         masm.store32(Imm32(JSString::DEPENDENT_FLAGS | JSString::LATIN1_CHARS_BIT),
                      Address(output, JSString::offsetOfFlags()));
-        masm.loadPtr(Address(string, JSString::offsetOfNonInlineChars()), temp);
+        masm.loadNonInlineStringChars(string, temp);
         static_assert(sizeof(char) == 1, "begin index shouldn't need scaling");
         masm.addPtr(begin, temp);
-        masm.storePtr(temp, Address(output, JSString::offsetOfNonInlineChars()));
+        masm.storeNonInlineStringChars(temp, output);
         masm.jump(done);
     }
 
     masm.bind(done);
 }
 
 JitCode*
 JitCompartment::generateStringConcatStub(JSContext* cx)
@@ -8037,18 +8035,17 @@ JitCompartment::generateStringConcatStub
     // lhs and rhs flags, so we just have to clear the other flags to get our
     // rope flags (Latin1 if both lhs and rhs are Latin1).
     static_assert(JSString::INIT_ROPE_FLAGS == 0, "Rope type flags must be 0");
     masm.and32(Imm32(JSString::LATIN1_CHARS_BIT), temp1);
     masm.store32(temp1, Address(output, JSString::offsetOfFlags()));
     masm.store32(temp2, Address(output, JSString::offsetOfLength()));
 
     // Store left and right nodes.
-    masm.storePtr(lhs, Address(output, JSRope::offsetOfLeft()));
-    masm.storePtr(rhs, Address(output, JSRope::offsetOfRight()));
+    masm.storeRopeChildren(lhs, rhs, output);
     masm.ret();
 
     masm.bind(&leftEmpty);
     masm.mov(rhs, output);
     masm.ret();
 
     masm.bind(&rightEmpty);
     masm.mov(lhs, output);
@@ -8335,34 +8332,32 @@ CodeGenerator::visitFromCodePoint(LFromC
         Label isSupplementary;
         masm.branch32(Assembler::AboveOrEqual, codePoint, Imm32(unicode::NonBMPMin),
                       &isSupplementary);
         {
             // Store length.
             masm.store32(Imm32(1), Address(output, JSString::offsetOfLength()));
 
             // Load chars pointer in temp1.
-            masm.computeEffectiveAddress(Address(output, JSInlineString::offsetOfInlineStorage()),
-                                         temp1);
+            masm.loadInlineStringCharsForStore(output, temp1);
 
             masm.store16(codePoint, Address(temp1, 0));
 
             // Null-terminate.
             masm.store16(Imm32(0), Address(temp1, sizeof(char16_t)));
 
             masm.jump(done);
         }
         masm.bind(&isSupplementary);
         {
             // Store length.
             masm.store32(Imm32(2), Address(output, JSString::offsetOfLength()));
 
             // Load chars pointer in temp1.
-            masm.computeEffectiveAddress(Address(output, JSInlineString::offsetOfInlineStorage()),
-                                         temp1);
+            masm.loadInlineStringCharsForStore(output, temp1);
 
             // Inlined unicode::LeadSurrogate(uint32_t).
             masm.move32(codePoint, temp2);
             masm.rshift32(Imm32(10), temp2);
             masm.add32(Imm32(unicode::LeadSurrogateMin - (unicode::NonBMPMin >> 10)), temp2);
 
             masm.store16(temp2, Address(temp1, 0));
 
--- a/js/src/jit/MacroAssembler.cpp
+++ b/js/src/jit/MacroAssembler.cpp
@@ -1386,32 +1386,117 @@ MacroAssembler::loadStringChars(Register
     // If it's not an inline string, load the non-inline chars. Use a
     // conditional move to prevent speculative execution.
     test32LoadPtr(Assembler::Zero,
                   Address(str, JSString::offsetOfFlags()), Imm32(JSString::INLINE_CHARS_BIT),
                   Address(str, JSString::offsetOfNonInlineChars()), dest);
 }
 
 void
+MacroAssembler::loadNonInlineStringChars(Register str, Register dest)
+{
+    MOZ_ASSERT(str != dest);
+
+    if (JitOptions.spectreStringMitigations) {
+        movePtr(ImmWord(0), dest);
+
+        // First, if the string is a rope, zero the |str| register. The code
+        // below depends on str->flags so this should block speculative
+        // execution.
+        test32MovePtr(Assembler::Zero,
+                      Address(str, JSString::offsetOfFlags()), Imm32(JSString::LINEAR_BIT),
+                      dest, str);
+
+        // Load non-inline chars if the inline-chars bit is not set.
+        test32LoadPtr(Assembler::Zero,
+                      Address(str, JSString::offsetOfFlags()), Imm32(JSString::INLINE_CHARS_BIT),
+                      Address(str, JSString::offsetOfNonInlineChars()), dest);
+    } else {
+        loadPtr(Address(str, JSString::offsetOfNonInlineChars()), dest);
+    }
+}
+
+void
+MacroAssembler::storeNonInlineStringChars(Register chars, Register str)
+{
+    MOZ_ASSERT(chars != str);
+    storePtr(chars, Address(str, JSString::offsetOfNonInlineChars()));
+}
+
+void
+MacroAssembler::loadInlineStringCharsForStore(Register str, Register dest)
+{
+    computeEffectiveAddress(Address(str, JSInlineString::offsetOfInlineStorage()), dest);
+}
+
+void
+MacroAssembler::loadInlineStringChars(Register str, Register dest)
+{
+    MOZ_ASSERT(str != dest);
+
+    if (JitOptions.spectreStringMitigations) {
+        // Making this Spectre-safe is a bit complicated: using
+        // computeEffectiveAddress and then zeroing the output register if
+        // non-inline is not sufficient: when the index is very large, it would
+        // allow reading |nullptr + index|. Just fall back to loadStringChars
+        // for now.
+        loadStringChars(str, dest);
+    } else {
+        computeEffectiveAddress(Address(str, JSInlineString::offsetOfInlineStorage()), dest);
+    }
+}
+
+void
 MacroAssembler::loadRopeLeftChild(Register str, Register dest)
 {
     MOZ_ASSERT(str != dest);
 
     if (JitOptions.spectreStringMitigations) {
         // Zero the output register if the input was not a rope.
         movePtr(ImmWord(0), dest);
         test32LoadPtr(Assembler::Zero,
                       Address(str, JSString::offsetOfFlags()), Imm32(JSString::LINEAR_BIT),
                       Address(str, JSRope::offsetOfLeft()), dest);
     } else {
         loadPtr(Address(str, JSRope::offsetOfLeft()), dest);
     }
 }
 
 void
+MacroAssembler::storeRopeChildren(Register left, Register right, Register str)
+{
+    storePtr(left, Address(str, JSRope::offsetOfLeft()));
+    storePtr(right, Address(str, JSRope::offsetOfRight()));
+}
+
+void
+MacroAssembler::loadDependentStringBase(Register str, Register dest)
+{
+    MOZ_ASSERT(str != dest);
+
+    if (JitOptions.spectreStringMitigations) {
+        // If the string does not have a base-string, zero the |str| register.
+        // The code below loads str->base so this should block speculative
+        // execution.
+        movePtr(ImmWord(0), dest);
+        test32MovePtr(Assembler::Zero,
+                      Address(str, JSString::offsetOfFlags()), Imm32(JSString::HAS_BASE_BIT),
+                      dest, str);
+    }
+
+    loadPtr(Address(str, JSDependentString::offsetOfBase()), dest);
+}
+
+void
+MacroAssembler::storeDependentStringBase(Register base, Register str)
+{
+    storePtr(base, Address(str, JSDependentString::offsetOfBase()));
+}
+
+void
 MacroAssembler::loadStringChar(Register str, Register index, Register output, Register scratch,
                                Label* fail)
 {
     MOZ_ASSERT(str != output);
     MOZ_ASSERT(str != index);
     MOZ_ASSERT(index != output);
     MOZ_ASSERT(output != scratch);
 
--- a/js/src/jit/MacroAssembler.h
+++ b/js/src/jit/MacroAssembler.h
@@ -1958,19 +1958,32 @@ class MacroAssembler : public MacroAssem
         loadPtr(Address(dest, ObjectGroup::offsetOfProto()), dest);
     }
 
     void loadStringLength(Register str, Register dest) {
         load32(Address(str, JSString::offsetOfLength()), dest);
     }
 
     void loadStringChars(Register str, Register dest);
+
+    void loadNonInlineStringChars(Register str, Register dest);
+    void loadNonInlineStringCharsForStore(Register str, Register dest);
+    void storeNonInlineStringChars(Register chars, Register str);
+
+    void loadInlineStringChars(Register str, Register dest);
+    void loadInlineStringCharsForStore(Register str, Register dest);
+
     void loadStringChar(Register str, Register index, Register output, Register scratch,
                         Label* fail);
+
     void loadRopeLeftChild(Register str, Register dest);
+    void storeRopeChildren(Register left, Register right, Register str);
+
+    void loadDependentStringBase(Register str, Register dest);
+    void storeDependentStringBase(Register base, Register str);
 
     void loadStringIndexValue(Register str, Register dest, Label* fail);
 
     void loadJSContext(Register dest);
     void loadJitActivation(Register dest) {
         loadJSContext(dest);
         loadPtr(Address(dest, offsetof(JSContext, activation_)), dest);
     }
--- a/js/src/vm/String.h
+++ b/js/src/vm/String.h
@@ -511,23 +511,28 @@ class JSString : public js::gc::TenuredC
 
     static size_t offsetOfLength() {
         return offsetof(JSString, d.u1.length);
     }
     static size_t offsetOfFlags() {
         return offsetof(JSString, d.u1.flags);
     }
 
+  private:
+    // To help avoid writing Spectre-unsafe code, we only allow MacroAssembler
+    // to call the method below.
+    friend class js::jit::MacroAssembler;
     static size_t offsetOfNonInlineChars() {
         static_assert(offsetof(JSString, d.s.u2.nonInlineCharsTwoByte) ==
                       offsetof(JSString, d.s.u2.nonInlineCharsLatin1),
                       "nonInlineCharsTwoByte and nonInlineCharsLatin1 must have same offset");
         return offsetof(JSString, d.s.u2.nonInlineCharsTwoByte);
     }
 
+  public:
     static const JS::TraceKind TraceKind = JS::TraceKind::String;
 
 #ifdef DEBUG
     void dump(); // Debugger-friendly stderr dump.
     void dump(js::GenericPrinter& out);
     void dumpNoNewline(js::GenericPrinter& out);
     void dumpCharsNoNewline(js::GenericPrinter& out);
     void dumpRepresentation(js::GenericPrinter& out, int indent) const;
@@ -605,26 +610,31 @@ class JSRope : public JSString
 
     JSString* rightChild() const {
         MOZ_ASSERT(isRope());
         return d.s.u3.right;
     }
 
     void traceChildren(JSTracer* trc);
 
+#ifdef DEBUG
+    void dumpRepresentation(js::GenericPrinter& out, int indent) const;
+#endif
+
+  private:
+    // To help avoid writing Spectre-unsafe code, we only allow MacroAssembler
+    // to call the methods below.
+    friend class js::jit::MacroAssembler;
+
     static size_t offsetOfLeft() {
         return offsetof(JSRope, d.s.u2.left);
     }
     static size_t offsetOfRight() {
         return offsetof(JSRope, d.s.u3.right);
     }
-
-#ifdef DEBUG
-    void dumpRepresentation(js::GenericPrinter& out, int indent) const;
-#endif
 };
 
 static_assert(sizeof(JSRope) == sizeof(JSString),
               "string subclasses must be binary-compatible with JSString");
 
 class JSLinearString : public JSString
 {
     friend class JSString;
@@ -737,23 +747,28 @@ class JSDependentString : public JSLinea
         MOZ_ASSERT(offset < base()->length());
         return mozilla::Some(offset);
     }
 
   public:
     static inline JSLinearString* new_(JSContext* cx, JSLinearString* base,
                                        size_t start, size_t length);
 
+#ifdef DEBUG
+    void dumpRepresentation(js::GenericPrinter& out, int indent) const;
+#endif
+
+  private:
+    // To help avoid writing Spectre-unsafe code, we only allow MacroAssembler
+    // to call the method below.
+    friend class js::jit::MacroAssembler;
+
     inline static size_t offsetOfBase() {
         return offsetof(JSDependentString, d.s.u3.base);
     }
-
-#ifdef DEBUG
-    void dumpRepresentation(js::GenericPrinter& out, int indent) const;
-#endif
 };
 
 static_assert(sizeof(JSDependentString) == sizeof(JSString),
               "string subclasses must be binary-compatible with JSString");
 
 class JSFlatString : public JSLinearString
 {
     /* Vacuous and therefore unimplemented. */
@@ -875,23 +890,27 @@ class JSInlineString : public JSFlatStri
         MOZ_ASSERT(JSString::isInline());
         MOZ_ASSERT(hasTwoByteChars());
         return d.inlineStorageTwoByte;
     }
 
     template<typename CharT>
     static bool lengthFits(size_t length);
 
+#ifdef DEBUG
+    void dumpRepresentation(js::GenericPrinter& out, int indent) const;
+#endif
+
+  private:
+    // To help avoid writing Spectre-unsafe code, we only allow MacroAssembler
+    // to call the method below.
+    friend class js::jit::MacroAssembler;
     static size_t offsetOfInlineStorage() {
         return offsetof(JSInlineString, d.inlineStorageTwoByte);
     }
-
-#ifdef DEBUG
-    void dumpRepresentation(js::GenericPrinter& out, int indent) const;
-#endif
 };
 
 static_assert(sizeof(JSInlineString) == sizeof(JSString),
               "string subclasses must be binary-compatible with JSString");
 
 /*
  * On 32-bit platforms, JSThinInlineString can store 7 Latin1 characters or 3
  * TwoByte characters (excluding null terminator) inline. On 64-bit platforms,