[PR #5918] [MERGED] fix(model): admit safetensors header lengths that resemble pickle markers #7106

Closed
opened 2026-11-01 21:45:26 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5918
Author: @amyanger
Created: 10/21/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: fix/5895-safetensors-pickle-sniff


📝 Commits (1)

  • 9ac6d48 fix(model): admit safetensors header lengths that resemble pickle markers

📊 Changes

4 files changed (+94 additions, -9 deletions)

View changed files

📝 src/model/ModelArtifactProvider.ts (+2 -8)
📝 src/model/ModelFormat.ts (+7 -0)
📝 src/model/SafetensorsHeader.ts (+11 -1)
📝 tests/boundary/model/modelArtifact.test.ts (+74 -0)

📄 Description

Summary

Fixes #5895.

inspect_model_artifact refused valid safetensors files whose JSON header length is 640, 896, 1152, 1408, 66176 and so on. Those lengths encode as 80 02..05 ... in the 8-byte little-endian prefix, which the provider read as a pickle PROTO marker. The CLI exited 1 with unsupported_target and asked for safetensors, although the input already was safetensors. These files now parse. Genuine pickles are still refused as unsupported and are never deserialized.

Problem and expected behavior

Smallest trigger: a safetensors file with one F32 tensor whose header is padded to 640 bytes. Its prefix is 80 02 00 00 00 00 00 00 followed by {. The provider checked only the first two bytes (0x80, then 2 to 5) and threw the pickle refusal before the safetensors parser ran.

The safetensors format defines an 8-byte unsigned little-endian length followed by UTF-8 JSON that begins with {. Nothing in it excludes these lengths, so a file with that structure should be parsed.

Change and scope

The pickle decision moved from ModelArtifactProvider.#read into readSafetensorsHeader, the parser that already owns the length prefix and the { check. For a pickle-like prefix it:

  • refuses as pickle when the file is shorter than 9 bytes, so no other error fires first;
  • reads bytes 4 to 8 through the existing ModelHeaderReader, which is hashed, abort-checked and loops on short reads;
  • refuses as pickle unless byte 8 is { and the declared length is within MODEL_HEADER_BYTES (100,000,000), the safetensors header ceiling the parser already enforces.

A file that passes continues through the normal parser. So a truncated safetensors file with a colliding length now gets the existing file_format_rejected error for an out-of-file header instead of the pickle diagnosis. The refusal message moved into a shared rejectPickle() in ModelFormat.ts, used by the ZIP check and the parser. Its text is unchanged.

Precedent: the parser already rejects any non-{ byte 8 and any length beyond MODEL_HEADER_BYTES. The tiebreak uses those same facts rather than a new heuristic.

Not included: the ZIP magic 50 4B 03 04 collides the same way with exactly one safetensors header length, 67,324,752. #5895 does not report it, so I left it out to keep this PR to one outcome. It can be a follow-up issue.

Scope decision: not applicable. This is a fix inside the existing model-artifact boundary.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/model/SafetensorsHeader.ts (readSafetensorsHeader), at format detection.
  • Contribution scope fit and linked maintainer decision: ordinary fix, not applicable.
  • CLI and MCP/tool-catalog contract: no schema change. Some inputs refused before now return a normal safetensors result. A truncated safetensors file with a colliding length now returns file_format_rejected instead of unsupported_target.
  • Provider, bridge, target-format, or platform compatibility: none beyond the above.
  • Evidence, artifact, provenance, or reconstruction contract: unchanged. Partial Evidence exists only on the GGUF path, and digests and file identity are finalized only after a successful parse.
  • Target selection, authority, upstream behavior, and cleanup details: not applicable.
  • Generated metadata, package, or installation impact: none.

Expected effects:

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added in tests/boundary/model/modelArtifact.test.ts:
    • safetensors with header lengths 640, 896, 1152, 1408 and 66176 is admitted with its weight tensor;
    • pickle refusal still holds for: a protocol 2 dict, a 6-byte protocol 2 {}, a protocol 4 FRAME pickle, the legacy torch.save magic, the protocol 2 pickle of "x{" (brace at byte 8, inadmissible length) and an in-file length with no brace;
    • a 500-byte truncation of an 896-length safetensors file is reported as BinaryTargetError file_format_rejected, not pickle.
  • Base reproduction (executed). Built CLI at base d672cc239 with the issue's nine-case script: lengths 640, 896, 1152 and 1408 exit 1 with unsupported_target and PyTorch pickle/ZIP model deserialization is unsupported; supply GGUF or safetensors instead. Lengths 632, 648, 904, 1160 and 1416 parse.
  • Final tests against base source (executed, new test file on base src/model): 6 failed, 74 passed. The failures are the five admission cases and the truncation case. The refusal cases pass on base, as they should.
  • User-visible CLI output after the fix (executed): all nine issue lengths print "exit":0,"format":"safetensors". Real Python pickles at protocols 2, 3, 4 and 5 and pickle.dumps('x{', 2) print unsupported_target.
  • Mutation (executed, each mutant compiled with 0 TS errors and failed on assertions):
    • remove the short-file guard: 1 test fails;
    • short-file guard < 9 to < 5: 1 fails (the 6-byte {} pickle);
    • always refuse a pickle-like prefix (base behavior): 6 fail;
    • never treat the prefix as pickle: 6 fail;
    • remove the header-ceiling condition: 1 fails ("x{");
    • invert the header-ceiling condition: 7 fail.
      The > to >= mutant at the ceiling is equivalent. 100,000,000 is 0x05F5E100, whose low byte is 0x00, so no pickle-like prefix can equal it.
  • Remaining proof gaps:
    • A crafted pickle can still be parsed as safetensors and get a format error instead of unsupported. It needs byte 3 at most 5, bytes 4 to 7 zero and byte 8 {, for example pickle.dumps('\x00{', 2) or pickle.dumps(b'\x00\x00\x00\x00{', 3). Those bytes are also a valid safetensors prefix, so no prefix rule can classify both. Nothing is deserialized either way. A reviewer ran about 810,000 generated pickles at protocols 2 to 5 (state-dict shapes, nested objects, strings full of {, the torch magic) through the rule. Only this crafted class was routed to safetensors. Protocol 4 and 5 cannot be: byte 8 is a high byte of the FRAME length.
    • A pickle-prefixed file of 5 to 8 bytes is still called a pickle, as on base, even if it is a truncated safetensors file.
    • ZIP collision at length 67,324,752, described above.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Development base: d672cc23998b6cbb21c485f118595ebfb82fed33, the newest green upstream main at the time; every base comparison below used it. Before publishing, the branch was rebased onto the newer green main c76edbebbe1e25c2bf821ec04d08dc851e37626c, which changes nothing under src/model or tests/boundary/model. After the rebase: the focused command below passed 90 tests, npm run check:fast passed and npm run test:changed -- --base c76edbebbe1e25c2bf821ec04d08dc851e37626c passed 3 tests.

  • npm run test:focused -- tests/boundary/model/modelArtifact.test.ts tests/acceptance/modelArtifactCli.test.ts tests/boundary/model/modelArtifactPartialCancellation.test.ts tests/boundary/mcp/modelArtifactPartialCancellation.test.ts: 4 files, 90 tests passed.
  • npm run check:fast: passed.
  • npm run test:changed -- --base d672cc23998b6cbb21c485f118595ebfb82fed33: 2 files, 3 tests passed. It selects source tests only; the built model boundary tests ran above.
  • npm run check: passed, including module boundaries and architecture guards. Last run before the test-only addition of the 6-byte pickle case; check:fast and the focused tests ran after it.
  • npx oxfmt --check src/model tests/boundary/model: passed.
  • npm run build:cached and then the issue's reproduction script through scripts/rea.mjs inspect-model-artifact: all nine lengths parse as safetensors.
  • Same built CLI on real Python pickles (protocols 2 to 5, and 'x{'): all unsupported_target.
  • npm run docs:check and npm run verify:test-discovery: not run. No contract, generated metadata or new test files.

Review:

  • Adversarial review, round 1: one finding, a crafted pickle routed to the parser. Recorded as the gap above.
  • code-review (high), round 1: the first version used a raw file.read side probe that skipped short-read looping and abort checks and duplicated the length decode. Fixed by moving the decision into the parser, through the reader.
  • Round 2: a truncated safetensors file with a colliding length got the pickle diagnosis. Fixed. The 648 case did not exercise the collision, so it was replaced with 66176.
  • Round 3: the protocol 2 "x{" pickle was misrouted. Resolved with the header-ceiling condition and a test. The human owner chose that tiebreak over two alternatives.
  • Round 4: the ZIP collision (recorded above as out of scope) and the short-guard mutant (fixed with the 6-byte case).
  • Declined across rounds:
    • moving all magic sniffing into one helper, because only safetensors' own length prefix creates the overlap;
    • removing first[1] ?? 0, which noUncheckedIndexedAccess requires;
    • folding the short-file guard into the brace read, because files of 4 to 7 bytes would still fail at take(4) first;
    • a docs change, because docs/model-artifacts.md only says pickle deserialization is unsupported, which still holds.
  • Final adversarial round: no findings.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Previously refused valid files now parse. One error class changes, for truncated colliding safetensors files, as described above.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable. The model reader is pure Node, and the real path ran through the built CLI on macOS.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: pickle bytes are still never deserialized. The extra bytes read before a refusal (at most 5) go through the same bounded reader and are not emitted.

Author readiness

Human owner: @amyanger

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5918 **Author:** [@amyanger](https://github.com/amyanger) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/5895-safetensors-pickle-sniff` --- ### 📝 Commits (1) - [`9ac6d48`](https://github.com/morluto/rea/commit/9ac6d485f4ec2fa5fcd99118fef514804cdf5fff) fix(model): admit safetensors header lengths that resemble pickle markers ### 📊 Changes **4 files changed** (+94 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `src/model/ModelArtifactProvider.ts` (+2 -8) 📝 `src/model/ModelFormat.ts` (+7 -0) 📝 `src/model/SafetensorsHeader.ts` (+11 -1) 📝 `tests/boundary/model/modelArtifact.test.ts` (+74 -0) </details> ### 📄 Description ## Summary Fixes #5895. `inspect_model_artifact` refused valid safetensors files whose JSON header length is 640, 896, 1152, 1408, 66176 and so on. Those lengths encode as `80 02..05 ...` in the 8-byte little-endian prefix, which the provider read as a pickle PROTO marker. The CLI exited 1 with `unsupported_target` and asked for safetensors, although the input already was safetensors. These files now parse. Genuine pickles are still refused as unsupported and are never deserialized. ## Problem and expected behavior Smallest trigger: a safetensors file with one `F32` tensor whose header is padded to 640 bytes. Its prefix is `80 02 00 00 00 00 00 00` followed by `{`. The provider checked only the first two bytes (`0x80`, then 2 to 5) and threw the pickle refusal before the safetensors parser ran. The safetensors format defines an 8-byte unsigned little-endian length followed by UTF-8 JSON that begins with `{`. Nothing in it excludes these lengths, so a file with that structure should be parsed. ## Change and scope The pickle decision moved from `ModelArtifactProvider.#read` into `readSafetensorsHeader`, the parser that already owns the length prefix and the `{` check. For a pickle-like prefix it: - refuses as pickle when the file is shorter than 9 bytes, so no other error fires first; - reads bytes 4 to 8 through the existing `ModelHeaderReader`, which is hashed, abort-checked and loops on short reads; - refuses as pickle unless byte 8 is `{` and the declared length is within `MODEL_HEADER_BYTES` (100,000,000), the safetensors header ceiling the parser already enforces. A file that passes continues through the normal parser. So a truncated safetensors file with a colliding length now gets the existing `file_format_rejected` error for an out-of-file header instead of the pickle diagnosis. The refusal message moved into a shared `rejectPickle()` in `ModelFormat.ts`, used by the ZIP check and the parser. Its text is unchanged. Precedent: the parser already rejects any non-`{` byte 8 and any length beyond `MODEL_HEADER_BYTES`. The tiebreak uses those same facts rather than a new heuristic. Not included: the ZIP magic `50 4B 03 04` collides the same way with exactly one safetensors header length, 67,324,752. #5895 does not report it, so I left it out to keep this PR to one outcome. It can be a follow-up issue. Scope decision: not applicable. This is a fix inside the existing model-artifact boundary. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/model/SafetensorsHeader.ts` (`readSafetensorsHeader`), at format detection. - Contribution scope fit and linked maintainer decision: ordinary fix, not applicable. - CLI and MCP/tool-catalog contract: no schema change. Some inputs refused before now return a normal safetensors result. A truncated safetensors file with a colliding length now returns `file_format_rejected` instead of `unsupported_target`. - Provider, bridge, target-format, or platform compatibility: none beyond the above. - Evidence, artifact, provenance, or reconstruction contract: unchanged. Partial Evidence exists only on the GGUF path, and digests and file identity are finalized only after a successful parse. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added in `tests/boundary/model/modelArtifact.test.ts`: - safetensors with header lengths 640, 896, 1152, 1408 and 66176 is admitted with its `weight` tensor; - pickle refusal still holds for: a protocol 2 dict, a 6-byte protocol 2 `{}`, a protocol 4 FRAME pickle, the legacy `torch.save` magic, the protocol 2 pickle of `"x{"` (brace at byte 8, inadmissible length) and an in-file length with no brace; - a 500-byte truncation of an 896-length safetensors file is reported as `BinaryTargetError` `file_format_rejected`, not pickle. - Base reproduction (executed). Built CLI at base `d672cc239` with the issue's nine-case script: lengths 640, 896, 1152 and 1408 exit 1 with `unsupported_target` and `PyTorch pickle/ZIP model deserialization is unsupported; supply GGUF or safetensors instead`. Lengths 632, 648, 904, 1160 and 1416 parse. - Final tests against base source (executed, new test file on base `src/model`): 6 failed, 74 passed. The failures are the five admission cases and the truncation case. The refusal cases pass on base, as they should. - User-visible CLI output after the fix (executed): all nine issue lengths print `"exit":0,"format":"safetensors"`. Real Python pickles at protocols 2, 3, 4 and 5 and `pickle.dumps('x{', 2)` print `unsupported_target`. - Mutation (executed, each mutant compiled with 0 TS errors and failed on assertions): - remove the short-file guard: 1 test fails; - short-file guard `< 9` to `< 5`: 1 fails (the 6-byte `{}` pickle); - always refuse a pickle-like prefix (base behavior): 6 fail; - never treat the prefix as pickle: 6 fail; - remove the header-ceiling condition: 1 fails (`"x{"`); - invert the header-ceiling condition: 7 fail. The `>` to `>=` mutant at the ceiling is equivalent. 100,000,000 is `0x05F5E100`, whose low byte is `0x00`, so no pickle-like prefix can equal it. - Remaining proof gaps: - A crafted pickle can still be parsed as safetensors and get a format error instead of `unsupported`. It needs byte 3 at most 5, bytes 4 to 7 zero and byte 8 `{`, for example `pickle.dumps('\x00{', 2)` or `pickle.dumps(b'\x00\x00\x00\x00{', 3)`. Those bytes are also a valid safetensors prefix, so no prefix rule can classify both. Nothing is deserialized either way. A reviewer ran about 810,000 generated pickles at protocols 2 to 5 (state-dict shapes, nested objects, strings full of `{`, the torch magic) through the rule. Only this crafted class was routed to safetensors. Protocol 4 and 5 cannot be: byte 8 is a high byte of the FRAME length. - A pickle-prefixed file of 5 to 8 bytes is still called a pickle, as on base, even if it is a truncated safetensors file. - ZIP collision at length 67,324,752, described above. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Development base: `d672cc23998b6cbb21c485f118595ebfb82fed33`, the newest green upstream `main` at the time; every base comparison below used it. Before publishing, the branch was rebased onto the newer green `main` `c76edbebbe1e25c2bf821ec04d08dc851e37626c`, which changes nothing under `src/model` or `tests/boundary/model`. After the rebase: the focused command below passed 90 tests, `npm run check:fast` passed and `npm run test:changed -- --base c76edbebbe1e25c2bf821ec04d08dc851e37626c` passed 3 tests. - `npm run test:focused -- tests/boundary/model/modelArtifact.test.ts tests/acceptance/modelArtifactCli.test.ts tests/boundary/model/modelArtifactPartialCancellation.test.ts tests/boundary/mcp/modelArtifactPartialCancellation.test.ts`: 4 files, 90 tests passed. - `npm run check:fast`: passed. - `npm run test:changed -- --base d672cc23998b6cbb21c485f118595ebfb82fed33`: 2 files, 3 tests passed. It selects source tests only; the built model boundary tests ran above. - `npm run check`: passed, including module boundaries and architecture guards. Last run before the test-only addition of the 6-byte pickle case; `check:fast` and the focused tests ran after it. - `npx oxfmt --check src/model tests/boundary/model`: passed. - `npm run build:cached` and then the issue's reproduction script through `scripts/rea.mjs inspect-model-artifact`: all nine lengths parse as safetensors. - Same built CLI on real Python pickles (protocols 2 to 5, and `'x{'`): all `unsupported_target`. - `npm run docs:check` and `npm run verify:test-discovery`: not run. No contract, generated metadata or new test files. Review: - Adversarial review, round 1: one finding, a crafted pickle routed to the parser. Recorded as the gap above. - `code-review` (high), round 1: the first version used a raw `file.read` side probe that skipped short-read looping and abort checks and duplicated the length decode. Fixed by moving the decision into the parser, through the reader. - Round 2: a truncated safetensors file with a colliding length got the pickle diagnosis. Fixed. The 648 case did not exercise the collision, so it was replaced with 66176. - Round 3: the protocol 2 `"x{"` pickle was misrouted. Resolved with the header-ceiling condition and a test. The human owner chose that tiebreak over two alternatives. - Round 4: the ZIP collision (recorded above as out of scope) and the short-guard mutant (fixed with the 6-byte case). - Declined across rounds: - moving all magic sniffing into one helper, because only safetensors' own length prefix creates the overlap; - removing `first[1] ?? 0`, which `noUncheckedIndexedAccess` requires; - folding the short-file guard into the brace read, because files of 4 to 7 bytes would still fail at `take(4)` first; - a docs change, because `docs/model-artifacts.md` only says pickle deserialization is unsupported, which still holds. - Final adversarial round: no findings. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Previously refused valid files now parse. One error class changes, for truncated colliding safetensors files, as described above. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. The model reader is pure Node, and the real path ran through the built CLI on macOS. - Package or release metadata impact: none. - Security, privacy, process, or containment review: pickle bytes are still never deserialized. The extra bytes read before a refusal (at most 5) go through the same bounded reader and are not emitted. ## Author readiness Human owner: @amyanger - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:45:26 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#7106
No description provided.