fix(relinker): keep moved instructions off a function entry after padding

core/relinker/codegen/include/codegen/x86/X64OpcodeConstants.hpp

@@ -20,6 +20,7 @@ inline constexpr std::uint8_t PrefixAddressSize = 0x67;
 inline constexpr std::uint8_t RexMin = 0x40;
 inline constexpr std::uint8_t RexMax = 0x4F;
 inline constexpr std::uint8_t RexWBit = 0x08;
+inline constexpr std::uint8_t RexBBit = 0x01;
 
 inline constexpr std::uint8_t TwoByteOpcodeEscape = 0x0F;
 
@@ -108,6 +109,8 @@ inline constexpr std::uint8_t OneByteLoopMax = 0xE2;
 inline constexpr std::uint8_t OneByteInOutImm8Min = 0xE4;
 inline constexpr std::uint8_t OneByteInOutImm8Max = 0xE7;
 inline constexpr std::uint8_t OneByteInt = 0xCD;
+inline constexpr std::uint8_t OneByteInt3 = 0xCC;
+inline constexpr std::uint8_t OneByteNop = 0x90;
 inline constexpr std::uint8_t OneByteRetImm16 = 0xC2;
 inline constexpr std::uint8_t OneByteRetFarImm16 = 0xCA;
 inline constexpr std::uint8_t OneByteEnter = 0xC8;

core/relinker/codegen/src/Amd64OnlyConverter.cpp

@@ -112,6 +112,41 @@ class WindowPlacer {
         return hit != _branchTargets.end() && *hit < end;
     }
 
+    [[nodiscard]] bool _int3(const std::size_t index) const {
+        const auto bytes = _bytes(index);
+        return bytes.size() == 1 && bytes[0] == X64OpcodeConstants::OneByteInt3;
+    }
+
+    [[nodiscard]] bool _padding(const std::size_t index) const {
+        const auto info = _info(index);
+        if (info.IsTwoByteOpcode)
+            return info.Opcode == X64OpcodeConstants::TwoByteNopModRm;
+        if (info.Opcode != X64OpcodeConstants::OneByteNop)
+            return _int3(index);
+        const auto prefixes = _bytes(index).first(info.OpcodeOffset);
+        return (info.RexPrefix & X64OpcodeConstants::RexBBit) == 0 && std::find(prefixes.begin(), prefixes.end(), X64OpcodeConstants::PrefixRep) == prefixes.end();
+    }
+
+    [[nodiscard]] bool _mayBeEntered(const std::size_t index) const {
+        for (auto previous = index; previous > 0; --previous) {
+            if (_end(previous - 1) != _address(previous) || _blocksFollowing(previous - 1))
+                return true;
+            if (!_padding(previous - 1))
+                return false;
+        }
+        return true;
+    }
+
+    [[nodiscard]] bool _enteredWithin(const std::size_t first, const std::size_t last) const {
+        if (_entered(_address(first), _end(last)))
+            return true;
+        for (auto index = first + 1; index <= last; ++index) {
+            if (!_int3(index) && _mayBeEntered(index))
+                return true;
+        }
+        return false;
+    }
+
     [[nodiscard]] bool _blocksFollowing(const std::size_t index) const {
         const auto kind = _info(index).FlowKind;
         return kind != ControlFlowKind::Sequential && kind != ControlFlowKind::ConditionalBranch;
@@ -135,7 +170,7 @@ class WindowPlacer {
     }
 
     [[nodiscard]] std::optional<Placement> _relocate(const std::size_t index, const std::size_t first, const std::size_t last, const bool allowInPlace = false) const {
-        if (_entered(_address(first), _end(last)))
+        if (_enteredWithin(first, last))
             return std::nullopt;
         StubBodyBuilder body;
         Placement placement;
@@ -162,7 +197,7 @@ class WindowPlacer {
         using Amd64OnlySubstitutionTable::kJmpRel32;
         std::size_t head = 0;
         for (auto last = start; last < _matches.size(); ++last) {
-            if (!_movable(last, true) || _entered(_address(start), _end(last)))
+            if (!_movable(last, true) || _enteredWithin(start, last))
                 return std::nullopt;
             if (head == 0 && _end(last) - _address(start) >= kJmpRel32.Size)
                 head = static_cast<std::size_t>(_end(last) - _address(start));

core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp

@@ -1058,6 +1058,39 @@ void converterWideStoreWindows() {
     requireFailure([&] { (void)convert(withStore(far, tail)); }, "A 256-bit store whose movable neighbours are all beyond a short jump was kept");
 }
 
+void converterIslandIndirectEntries() {
+    const auto converter = Codegen::MakeAmd64OnlyConverter(Codegen::Amd64OnlyTarget::Rosetta);
+    const Bytes prologue = {0x55, 0x48, 0x89, 0xE5, 0x41, 0x57, 0x41, 0x56, 0x41, 0x55, 0x41, 0x54, 0x53, 0xC3};
+    for (const auto& padding : {Bytes{0xCC, 0xCC}, Bytes{0x66, 0x90}, Bytes{0x0F, 0x1F, 0x00}, Bytes{}}) {
+        Bytes text = {0xE8, 0x40, 0x00, 0x00, 0x00, 0xC5, 0xFC, 0x11, 0x07, 0xC3, 0xEB, 0xFD};
+        text.insert(text.end(), padding.begin(), padding.end());
+        const auto entry = static_cast<Domain::FileByteOffset>(0x200 + text.size());
+        text.insert(text.end(), prologue.begin(), prologue.end());
+        Bytes file(0x300 + text.size(), 0xCC);
+        std::copy(text.begin(), text.end(), file.begin() + 0x200);
+        const auto result = converter->Convert(file, {segmentHeader(text.size())});
+        require(result.Trampolines.size() == 2 && result.KeptCount == 0, "A 256-bit store before a function that only a pointer reaches was not lowered through an island");
+        for (const auto& site : result.Trampolines)
+            require(site.Offset >= entry || site.Offset + site.Length <= entry, "An island covered the entry of a function that follows a jump and its padding");
+        require(result.Trampolines[0].Offset == entry, "An island next to a function that follows a jump and its padding did not start at its entry");
+    }
+}
+
+void converterIslandInTrapPadding() {
+    const auto converter = Codegen::MakeAmd64OnlyConverter(Codegen::Amd64OnlyTarget::Rosetta);
+    Bytes text = {0xE8, 0x40, 0x00, 0x00, 0x00, 0xC5, 0xFC, 0x11, 0x07, 0xC3, 0xEB, 0xFD};
+    const Bytes padding(15, 0xCC);
+    text.insert(text.end(), padding.begin(), padding.end());
+    const auto entry = static_cast<Domain::FileByteOffset>(0x200 + text.size());
+    text.insert(text.end(), {0xE9, 0x00, 0x01, 0x00, 0x00});
+    Bytes file(0x300 + text.size(), 0xCC);
+    std::copy(text.begin(), text.end(), file.begin() + 0x200);
+    const auto result = converter->Convert(file, {segmentHeader(text.size())});
+    require(result.Trampolines.size() == 2 && result.KeptCount == 0, "A 256-bit store before int3 padding and a function that cannot move was not lowered through an island");
+    for (const auto& site : result.Trampolines)
+        require(site.Offset + site.Length <= entry, "An island in the int3 padding after a jump ran into the next function");
+}
+
 void converterClaimedFollower() {
     const auto converter = Codegen::MakeAmd64OnlyConverter(Codegen::Amd64OnlyTarget::Rosetta);
     for (const auto& [shortInstruction, name] : {std::pair{Bytes{0x0F, 0x53, 0xCA}, "RCPPS"}, std::pair{Bytes{0xF3, 0x0F, 0xC7, 0xF8}, "RDPID"}}) {
@@ -2413,6 +2446,8 @@ int main() {
         wideStoreSplit();
         converterWideStoreWindows();
         converterWideStoreIslandAcrossInt64Max();
+        converterIslandIndirectEntries();
+        converterIslandInTrapPadding();
         converterClaimedFollower();
         converterJumpTableTargets();
         converterReciprocal();