Report source file:line (or symbol+offset) in diagnostics (issue #74) - #82
Merged
Merged
Conversation
Sanitizer violations now say where in the source they happened:
invalid 1-byte write at 0x000032a0 (heap redzone)
from PC 0x00002b38 (at work:memtest.s:281)
and line 281 of that source is exactly the offending instruction.
Two sources, best first: a HUNK_DEBUG `LINE` block's (line, offset)
pairs, else the binary's HUNK_SYMBOL table as symbol+offset. The raw PC
is always kept alongside rather than replaced -- it is what a
disassembly needs, and sparse line data points at a statement rather
than a specific instruction.
Parsing is lazy: `parse`/`load` capture the raw HUNK_DEBUG and
HUNK_SYMBOL payloads cheaply and never interpret them, with structured
decoding happening on first query. That meant no signature changes and
no existing caller touched, and it keeps the common case from paying for
debug info that can dominate a file (LawBreaker is 776 bytes of which
456 is debug).
`ShadowMap::report_with` takes a resolver *closure* rather than reaching
for the information: a shadow map knows PCs and nothing about hunks,
load addresses or debug blocks, so the lookup stays on the CLI's side
and `crate::sanitize` gains no dependency on the loader. `report()`
still exists, delegating with a no-op resolver.
Investigating what real toolchains actually emit reshaped the scope
twice, and both findings are documented rather than glossed:
- SAS/C's `LINE` block (DEBUG=LINE) is byte-compatible with the one
PhxAss emits (LINEDEBUG) and with LawBreaker's, so one parser serves
three producers. SAS/C also emits OPTS and SRC6 blocks alongside.
- m68k-amigaos-gcc emits *untagged stabs* -- its debug payload begins
`00 00 00 4c 00 00 00 10 ff ff ff ff`, so where LINE/OPTS/SRC6 carry
ASCII it has none. gcc therefore gets no file:line from this, which
matters because gcc users are the audience that motivated the whole
sanitizer effort. They do get symbol+offset: a real gcc-built stack
smash now reports `found 0x41414141 ... (at ___main+0x3c)`. Stabs is
filed as follow-up work; an untagged block has no magic to dispatch
on, so identifying it safely is its own problem, and a wrong line
number is worse than none.
The magic dispatch is an explicit enum with a documented
unrecognised-and-skipped arm, so adding stabs is a match arm rather
than unpicking an `if`. A test uses gcc's real untagged byte sequence
to prove it is skipped cleanly rather than mistaken for LINE.
New fixtures/linetest carries real LINE data, built by PhxAss with
LINEDEBUG. It replaces tests pointing at a scratch file and at
LawBreaker (unvendorable under Enforcer's licence), both of which would
have silently skipped forever once those files vanished. It is
deliberately PhxAss-only with no generator, since amiga_asm.py cannot
emit debug hunks and a generator producing a debug-free binary would
look authoritative while lacking the entire point.
It also proved a real requirement: PhxAss's data-hunk LINE pairs are
*not* offset-monotonic (line 33 -> 0x0e, 36 -> 0x00, 37 -> 0x15), so
the parser must sort rather than trust file order. Assuming order there
would have produced wrong line numbers, not missing ones.
Documented limits: line data is sparse, so a PC between entries is
attributed to the earlier statement; and `static` functions never
appear in HUNK_SYMBOL, so a violation inside one falls to the nearest
exported symbol, which can be a surprising name with a large offset.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23
cargo fmt --all --check was failing on an assert! that rustfmt wants split across lines. Tests and clippy were green; this was purely formatting, added after the gate run that formatted everything else. The pre-push check missed it because it read $? through a pipe, which reports the exit status of the last command in the pipeline (head), not cargo's -- so a genuine exit 1 read as a pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #74. Sanitizer violations now say where in the source they happened:
Line 281 of that source is exactly
move.b #$ab,32(a2) ; the overrun write itself— verified, since a wrong line is worse than none.Two sources, best first: a
HUNK_DEBUGLINEblock's(line, offset)pairs, elseHUNK_SYMBOLassymbol+offset, else the bare address. The raw PC is always kept alongside rather than replaced — it's what a disassembly needs, and sparse line data points at a statement rather than a specific instruction.The headline result: it answers the question that started all this
A real gcc-built C program with a classic
strcpystack overflow:found 0x41414141is the ASCIIAAAAthat overflowed — exactly the diagnostic value protected by making the report rollup conditional in #79 — and(at ___main+0x3c)is the symbol fallback working on a binary with no line data at all.What investigating real toolchains changed
Scope shifted twice, and both findings are in the docs rather than glossed over:
LINEblock is byte-compatible with PhxAss's and LawBreaker's, so one parser serves three producers. (SAS/C also emitsOPTSandSRC6alongside; both ignored.)00 00 00 4c 00 00 00 10 ff ff ff ff— whereLINE/OPTS/SRC6carry ASCII, it has none. So-galone gives nofile:linehere, which matters because gcc users are the audience that motivated the whole sanitizer effort. They getsymbol+offsetinstead, as above. Stabs is follow-up work: an untagged block has no magic to dispatch on, so identifying it safely is its own problem.The magic dispatch is an explicit enum with a documented skip arm, so adding stabs is a match arm rather than unpicking an
if. A test uses gcc's real untagged bytes to prove it's skipped cleanly rather than mistaken forLINE.Notes for review
parse/loadcapture raw payloads cheaply and never interpret them; decoding happens on first query. No signature changes, no existing caller touched, and the common case doesn't pay for debug info that can dominate a file (LawBreaker is 776 bytes, 456 of it debug).report_withtakes a closure, not the debug info. A shadow map knows PCs and nothing about hunks or load addresses, so the lookup stays on the CLI side andsanitize.rsgains no loader dependency.report()still delegates with a no-op resolver.fixtures/linetestcarries realLINEdata (PhxAssLINEDEBUG). It replaces tests pointing at a scratch file and at LawBreaker (unvendorable under Enforcer's licence) — both would have silently skipped forever once those files vanished. Deliberately PhxAss-only with no generator, sinceamiga_asm.pycan't emit debug hunks and a generator producing a debug-free binary would look authoritative while lacking the point.0x0e, 36→0x00, 37→0x15), so the parser must sort rather than trust file order. Trusting it would have produced wrong line numbers, not missing ones.Documented limits
Line data is sparse, so a PC between entries is attributed to the earlier statement. And
staticfunctions never appear inHUNK_SYMBOL, so a violation inside one falls to the nearest exported symbol — in the gcc example above the smash is actually invictim, reported as___main+0x3c. Both are in the CLI reference as warnings rather than left to surprise someone.999 tests green, clippy clean,
mkdocs build --strictclean.Separate finding, worth its own issue
gcc's
mallocdoesn't reachAllocMem— libnix takes one chunk and sub-allocates viaexec.library/Allocatefrom its ownMemHeader(11Allocatecalls for onemalloc). So--sanitize's heap redzones don't covermallocin C programs; abuf[32]overflow on amalloc(32)goes undetected. Stack detection is unaffected (hence the result above). I'll file that separately — it's a real limit on what the heap detectors reach for the gcc audience.🤖 Generated with Claude Code
https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23