tac: read regular files instead of mmap to avoid SIGBUS on truncation#12971
tac: read regular files instead of mmap to avoid SIGBUS on truncation#12971DarthStrom wants to merge 1 commit into
Conversation
|
GNU testsuite comparison: |
22ebf56 to
bbc2585
Compare
Merging this PR will degrade performance by 3.99%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | sort_long_line[10000] |
406.9 µs | 423.8 µs | -3.99% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing DarthStrom:tac-sigbus-9748 (783be34) with main (f50adad)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
|
|
Also you should add co-authors if code is taken from other PRs. |
bbc2585 to
7676ce5
Compare
|
@oech3 thanks for the review! Co-authors (your second comment): added memmap2 — I dug into this and I'd argue we should keep it, but I've narrowed where it's used. The unsoundness in #9748 is specifically about mmapping a caller-controlled file: another process can truncate it while it's mapped and we take a SIGBUS. While checking this I noticed the original fix was actually incomplete — it only covered the file-argument path. I removed that direct-stdin The reason I'd prefer to keep it rather than just So removing |
Ok. Thankyou. We can use mmap in the case. |
tac memory-mapped regular files. If another process truncated such a file while it was mapped (e.g. during log rotation), accessing the now-invalid pages raised SIGBUS and killed the process. Read regular files into memory up front instead, so a concurrent truncation can no longer crash tac. The stdin path had the same hole by a different route: `tac < file` mapped the raw stdin fd -- the caller's regular file -- directly. Remove that direct-stdin mmap and always route stdin through a process-owned temp file (already used to bound memory on large stdin, see uutils#10094). The temp file is created unlinked, so no other process can truncate it and mapping it stays sound. Adds regression tests that truncate a file mid-read, via both an argument and stdin redirection, and assert tac is not killed by a signal. Fixes uutils#9748 Co-authored-by: easonysliu <easonysliu@tencent.com> Co-authored-by: Charlie Tonneslan <cst0520@gmail.com>
7676ce5 to
783be34
Compare
|
Can we use MAP_POPULATE, etc... instead of removing mmap? Also I'm considering to add sealed-memfd + mmap + splice to uucore. |
Fixes #9748
Problem
tacmemory-maps regular files. If another process truncates such a file while it is mapped — e.g. a log file being rotated — accessing the now-invalid pages raises SIGBUS and the process is killed ("Bus error (core dumped)"). Mapping a file whose backing store can change underneath us is inherently unsound.Reproducer on
main:This reproduces 12/12 runs for me.
Fix
Read regular files into memory up front with
read_to_endrather than memory-mapping them, so the bytes are copied into process-owned memory before scanning and a concurrent truncation can no longer fault. The now-unusedtry_mmap_file()helper is removed.stdinis unchanged: it still mmaps a process-owned temporary file, where external truncation is not a concern.After the fix the reproducer above no longer crashes (0/20 runs).
Test
Adds
test_tac_file_truncated_during_read_does_not_crash, which truncates a file whiletacis reading it and asserts the process is not killed by a signal.There's a useful asymmetry that makes this a stable regression guard: once
taccopies the bytes up front, a truncation race can never SIGBUS, so the test is reliably green on the fixed code regardless of timing — while still failing on the old mmap-based code. I verified this by reverting just the source fix and keeping the test: it fails 3/3 withsignal Some(7)(SIGBUS).
Credit
This builds on the earlier
read_to_endapproach in #11326 and #11464 (both uutils PRs), which stalled on the missing regression test that this PR supplies.Checklist
cargo fmtcleancargo clippycleancargo test --features tac --no-default-features --test tests test_tac— 32/32 pass