fix(driver-portable): guard GGUF tensor bounds to surface truncation instead of SIGBUS#423
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Do truncated or corrupted GGUF files come up often enough that we should plan around them? I don't have a great feel for how frequent this is. If so, I'm wondering if it make sense for Pie to just reject them. dealing with the problematic gguf binaries feels like out of pie's responsibility |
4b28594 to
eebaaf9
Compare
A GGUF whose declared tensor region runs past the on-disk size passes gguf_init_from_file (no_alloc=true validates only the metadata header) yet SIGBUSes on first tensor read — an uncatchable fault that kills the embedded driver thread with no surfaced cause. Bounds-guard every tensor extent against the mapped file size in the GGUFArchive constructor and throw a clean std::runtime_error so the failure reaches stderr.
eebaaf9 to
43bf942
Compare
2 participants
Problem
When the embedded portable driver loads a truncated or corrupt GGUF, the driver thread dies with a SIGBUS and the process exits with no surfaced cause.
GGUFArchivemaps the whole model file withmmapand hands out tensor pointers asmmap_base + data_offset + tensor_offset.gguf_init_from_fileruns withno_alloc=true, so it validates only the metadata header and never touches the tensor-data section. A GGUF whose declared tensor region runs past the on-disk size therefore passes init and then SIGBUSes on the first tensor access — anmmapread beyond the file's last mapped page (for example, thetoken_embddequant reads at load time).Because the embedded driver runs as a thread inside the engine process with
install_signal_handlers=0, that SIGBUS kills the whole process. It bypasses the C++run_inproctry/catch(which only catchesstd::exception) and the host's startup-exit detection (which only catches clean nonzero returns, not signals). The result is an opaque signal death with the real cause unsurfaced.Note: this is unrelated to
python_snapshot/ the Python WASM runtime — that path is guarded separately and is never reached on a missing runtime.Fix
Add a bounds check in the
GGUFArchiveconstructor, before any tensor is read: for each tensor, assertdata_offset + tensor_offset + nbytes <= mapped size(overflow-safe), otherwise throw astd::runtime_errornaming the offending tensor and the sizes.The throw propagates through the driver's
run_inproccatch, which prints[pie-driver-portable] fatal: <reason>to stderr and returns nonzero, so the cause now reaches the host instead of an opaque signal death. Covers single-file and sharded loads (each shard runs through the same constructor).Existence, magic, and header validation are already handled by
gguf_init_from_file; this closes the remaining truncation/bounds gap.Test
Adds
portable_gguf_archive: writes a valid one-tensor GGUF, truncates it into the data section, and asserts the constructor throws a "truncated or corrupt" error, while the intact file opens cleanly.Verification
portable_gguf_archiveC++ test: PASS, and mutation-proven (removing the guard makes the test fail rather than SIGBUS).cargo build -p pie-server --releasewithPIE_PORTABLE_METAL=1: PASS — the guard compiles in the real portable build and the engine binary links.