Fix segfault when a delay-slot instruction has no p-code - #288
Conversation
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS Validation record for head
Caveats, one line each:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #288 +/- ##
=======================================
Coverage 86.82% 86.82%
=======================================
Files 5 5
Lines 516 516
Branches 82 82
=======================================
Hits 448 448
Misses 26 26
Partials 42 42 ☔ View full report in Codecov by Harness. |
SleighBuilder::delaySlot points the builder at a ParserWalker on its own
stack frame and puts the previous one back only on the normal exit path.
When the delay-slot instruction has no p-code section, PcodeBuilder::build
throws UnimplError, the walker is destroyed with the frame, and the
builder is left holding that address. Sleigh::oneInstruction catches the
exception and describes it through exactly that pointer, reading a
ParserContext out of stack the handler has already reused, so
Context.translate segfaults on any delay-slot branch followed by an
instruction with no semantics. Eight bytes of SPARC are enough:
Context("sparc:BE:32:default").translate(bytes.fromhex("63748596a7b8c9da"))
SleighBuilder::appendCrossBuild saves and restores the walker the same
unguarded way around a build() that can throw.
Restore both through a scope object so an exception leaving either method
cannot outlive the walker it installed. The reported instruction is now
the branch rather than its delay slot, which matches the instruction
length UnimplError already carries.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
40c3774 to
2bbb34d
Compare
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS Two things worth knowing before this is judged, both from building the trees rather than reading them. This has a real-world instance now, not only the synthetic one. #289 makes that symptom disappear without fixing the defect, so this must not be judged on top of it. Built separately: On a and the #289 tree reports the same object at the same line, differing only in the frame slot's size, 160 bytes against 544 — because #289 grows So if #289 lands first, a test of this case will pass whether or not this branch is applied, and the use-after-return will still be there. |
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS
Before — each case takes the whole process down with SIGSEGV, after disassembly of the same bytes has already succeeded: pypcode master, 559aacdAfter — the failure is a catchable with this change, 2bbb34d |
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS New since the 25 Aug comment: the trigger is a named function in shipped OpenSSL.
Three more probes in that file end the same way: Of the 17,946 SPARC objects in our sweep, five contain those sequences — all four of them, in every case — and all five are OpenSSL from NetBSD 10.1: The shared library is a plain download:
On this branch those eight bytes raise This and #289 are both ours and both still open. The 25 Aug comment has the evidence on how the two interact; which one you merge is your call. session: sharpen |
1 participant
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS
Problem
Context.translatetakes the whole process down with SIGSEGV when a delay-slot branch's delay slot has no p-code. Eight bytes reach it —0x63748596, a sparccall, followed by0xa7b8c9da:Toy:BE:32:defaultdoes the same on0xf500(callds) ahead of0xa800, with or without a decodable instruction in front. Disassembling those bytes succeeds, and an instruction with no p-code is otherwise a catchableUnimplError, so nothing walking unknown bytes — CFG recovery over a blob, a language sweep — can survive this.Root cause
SleighBuilder::delaySlotinpypcode/sleigh/sleigh.ccinstalls a walker living on its own stack frame and puts the previous one back only after the loop:ParserWalker *tmp = walker; uintb olduniqueoffset = uniqueoffset; ... ParserWalker newwalker( pos ); walker = &newwalker; walker->baseState(); build(walker->getConstructor()->getTempl(),-1); // Build the whole delay slot ... walker = tmp; // Restore original context uniqueoffset = olduniqueoffset;buildthrowsUnimplErrorfor a constructor with no p-code section (pypcode/sleigh/semantics.cc:929). The throw skips both assignments, so the builder still points atnewwalkeronce that frame is gone — andSleigh::oneInstructiondescribes the error through exactly that pointer:appendCrossBuildcarries the same tail-only restore.Fix
Both functions restore through a scope object, so no exit path can leave the builder holding a destroyed walker:
The failure then names the branch at its own address, which is what the error's
instruction_length = fallOffsetalready describes —fallOffsetspans the branch and its delay slot:With a decodable instruction ahead of the branch, that instruction is kept and the block ends there:
translate 1 p-code op(s), 1 instruction(s) kept.appendCrossBuildgets the guard for the same invariant but has no reproducer here: Hexagon is the only shipped language using CROSSBUILD, and it has no delay slots.Both functions are vendored from Ghidra unchanged, and Ghidra master at
382b26c9a508604d165ed9b20e8fb41c2a6edfc7still carries the identical tail-only restore, so the defect is upstream too.Testing
TranslateTests::test_delay_slot_unimpl_failureandtest_partial_delay_slot_unimpl_failurecover the sparc and Toy encodings above, each alone and each behind a decodable instruction, asserting the reported address is the branch's own. Against a baseline build ofpypcode/sleigh/sleigh.ccthe run never reaches an assertion: pytest exits 139 withFatal Python error: Segmentation faultintest_delay_slot_unimpl_failure. On this branch, 2 passed and 4 subtests passed.Validation: #288 (comment)
session: sharpen