Repository navigation
fix(relinker): reject LOCK-prefixed MOVNTSS and MOVNTSD - #10713
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 50dc1c9 against current main. The matcher rejects LOCK only in the instruction prefix bytes before the opcode, while the regression guards against confusing an immediate/displacement byte 0xF0 with a prefix and covers both orders for MOVNTSS/MOVNTSD plus the failing file offset. git diff --check, the current-main conventions check, and merge-tree pass. The relinker test target builds with GCC 12.2, but the registered converter test aborts with SIGILL on this host before reporting results. Linux and ASAN hosted checks pass; Windows and macOS matrix jobs are still running. No blocking source issue found.
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on exact head 50dc1c9: the complete hosted relinker matrix is now green on Linux, Windows, macOS arm64 and macOS x86_64, with ASAN and policy checks also passing. The earlier review findings and local SIGILL limitation are unchanged; merge state is now CLEAN.
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on exact head 50dc1c9: the complete hosted relinker matrix is now green on Linux, Windows, macOS arm64 and macOS x86_64, with ASAN and policy checks also passing. The earlier review findings and local SIGILL limitation are unchanged; merge state is now CLEAN.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 50dc1c93c8f3095d533ab7cc0b7f64cb0f8cec87. The matcher checks for LOCK only in the decoded prefix span before the opcode, so an F0 displacement/immediate byte is not rejected; the regression cases cover both prefix orders for both mandatory prefixes and assert the original file offset. The change is narrowly placed before replacement generation. Hosted relinker, ASAN, Linux, Windows, macOS arm64 and x86_64 checks pass, with product jobs skipped by policy; the PR reports a native Ryzen SIGILL probe for all four LOCK forms and valid controls. Local amd64_only_converter compiled, but execution on this host terminated with SIGILL, so I could not independently run its assertions. git diff --check and conventions pass, and the PR is mergeable.
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed head 50dc1c9 merged onto upstream/main acbfd7c (2026-10-25); the merge is clean. It is also clean onto the current main 63a863d, whose 11 newer commits touch neither core/relinker nor tools.
Environment: Windows 11 Home 10.0.26200, Intel i9-12900H, WinLibs GCC 15.2.0 UCRT, CMake 4.3.1, Ninja 1.13.2, Python 3.13.9, -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Release.
Results
-
Full relinker-only suite on the merged tree: 261 tests, 259 pass,
long_pathsskipped. The one failure ispatch_advisor_unit. It fails the same way on plainmainon this machine, because the temp path contains a space (#10667), and it is unrelated.amd64_only_converterandamd64_only_windowspass. -
Counter-check: with
main'sAmd64OnlyInstructionMatcher.cppunder this head's test file,amd64_only_converterfails withLOCK MOVNTSS/MOVNTSD was accepted. The harness stops at the first failure, so I also linked the converter once with each matcher and ran the encodings one at a time throughConvert, for both the Intel and the Rosetta target.mainaccepts all four test encodings and writesf0 f3 0f 11 07,f3 f0 0f 11 07,f0 f2 0f 11 07andf2 f0 0f 11 07. This branch rejects each one withMOVNTSS/MOVNTSD with a LOCK prefixat 0x20F. It also rejects LOCK before or after a REX byte (f0 f3 41 0f 2b 07,f3 41 f0 0f 2b 07), LOCK combined with a segment or address-size prefix, a doubled LOCK and a RIP-relative operand. Both builds still rewrite in place an F0 byte in the displacement or SIB (f3 0f 2b 47 f0,f3 0f 2b 04 f0,f3 0f 2b 87 f0 f0 f0 f0). -
End to end (
--to-intel --skip-syscall-check --skip-sce-module, code at 0x4000): forf0 f3 0f 2b 07,mainreportsIntel substitution: MOVNTSS at 0x4000 (5 bytes) -> in place 5 bytesand writesf0 f3 0f 11 07. This branch exits 2 withFAIL: MOVNTSS/MOVNTSD with a LOCK prefix (offset 0x4000), and the other three orders behave the same way. The unlocked forms and the F0-displacement form still convert tof3 0f 11 07,f2 0f 11 07andf3 0f 11 47 f0. -
Native probe on this Intel CPU (one child process per sequence, xmm0 stored to a valid buffer): the four
mainoutputs, which objdump shows aslock movssandlock movsd, raiseSTATUS_ILLEGAL_INSTRUCTION(0xc000001d).movssandmovsdwithout LOCK execute and store 4 and 8 bytes. So the description's "still faults" holds on Intel. The AMD side rests on your Ryzen runs. -
Mutants of the new guard, each run against this test file:
- Scanning the whole instruction fails at the new displacement case.
- Checking only the first byte gets past
f0 f3but fails atf3 f0. - Checking only the byte before the opcode, or skipping the first prefix byte, fails at
f0 f3.
So both prefix orders and the displacement case each catch a different wrong fix. Returning
Unsupportedinstead of throwing passes, with the same rejection and offset but the messageAMD-only instruction without Intel lowering: MOVNTSS(orMOVNTSD). That is an equivalent alternative, not a gap. -
The converter is the only production caller of the matcher. Its first pass (
Amd64OnlyConverter.cpp:334onmain, a file this PR does not change) runsMatchinside_atFileOffseton every scanned instruction before any placement code. The error therefore carries the instruction's own offset, and the follower and sequence paths cannot be the first to see a LOCK MOVNT. -
check_conventions.py --base upstream/mainpasses with this title and body, andgit diff --checkis clean. #8042, #8236 and #9184 already conflict withmain, and none of their conflict hunks with this head contain its lines. Hosted relinker jobs (ubuntu, windows, both macOS) and ASan are green on 50dc1c9.
Not checked: AMD hardware (your Ryzen probes cover that side). Linux and macOS locally: on MinGW the execution tests in this file are compiled out (#if (defined(__linux__) || defined(__APPLE__)) && defined(__x86_64__)), so my Windows run covers the matcher and converter assertions only. The Rosetta target only through the converter probe above, not with a macOS run.
I found nothing to change. The guard scans exactly the prefix run that IsMovntss and IsMovntsd skip. It therefore refuses LOCK in every position and never mistakes an operand byte for LOCK, and the tests catch each partial fix I tried. It is also the coverage oneandonlydean asked for on #10340: a prefix-order regression for each MOVNT form at converter level, with the failing offset. For disclosure: I suggested this guard on #10340 (the inline comment on Sse4aOperands.cpp), and I have open relinker PRs (#10752, #10732, #10731, #10730, #10712, #10679, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390) plus #10667 on tools/tests/test_suggest_patches.py; none touches core/relinker/codegen/.
Written with AI assistance (Claude Code). I ran the builds, tests, probes and end-to-end conversions above on my Windows 11 machine.
What
LOCK-prefixed MOVNTSS/MOVNTSD currently convert successfully into LOCK-prefixed MOVSS/MOVSD, leaving invalid code that still faults. Reject these encodings in the in-place matcher before generating replacement bytes.
F0.This is the separate MOVNT follow-up requested in #10340. That PR handles SHA-1, SHA-256, EXTRQ and INSERTQ; the patches are independent and can land in either order.
Tested
Linux x86-64, GCC 15.2.0, Release relinker-only build. The new converter regression fails against unchanged main production code with
LOCK MOVNTSS/MOVNTSD was acceptedand passes with the matcher guard. The complete relinker-only suite passes.Native AMD Ryzen 5 7640HS probes: MOVNTSS and MOVNTSD with LOCK before or after their mandatory prefix each raise SIGILL. All four controls without LOCK execute successfully, using valid destination buffers in separate processes. Windows and macOS execution were not rerun for this follow-up.
Manual inspection of the shared-file warnings for #8042, #8236 and #9184 found disjoint scanner and Rosetta execution tests; none changes the MOVNT matcher or these converter regressions.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)