Skip to content

Validate safetensors data offsets against file boundaries#3410

Merged
angeloskath merged 7 commits into
ml-explore:mainfrom
matinsaurralde:fix/safetensors-file-boundary-validation
Apr 15, 2026
Merged

Validate safetensors data offsets against file boundaries#3410
angeloskath merged 7 commits into
ml-explore:mainfrom
matinsaurralde:fix/safetensors-file-boundary-validation

Conversation

@matinsaurralde

Copy link
Copy Markdown
Contributor

Proposed changes

Addresses #3409.

The SafeTensors loader reads data_offsets from JSON metadata without checking whether they fall within the actual file. A crafted .safetensors file can declare offsets that exceed the file size, causing an out-of-bounds memory read when the tensor is evaluated.

This is the file-boundary counterpart to #3364 (which validates internal consistency between data_offsets and shape * dtype). The Rust reference implementation enforces both checks (TensorInvalidInfo and MetadataIncompleteBuffer).

Changes

mlx/io/safetensors.cpp — After parsing the JSON header, compute the available data size from the actual file size and validate each tensor's data_offsets:

  • data_offsets has at least 2 elements
  • begin <= end
  • end does not exceed the data region of the file

mlx/io/load.hParallelFileReader::seek() and FileWriter::seek() did not handle std::ios_base::end, silently falling through to SEEK_CUR. Added SEEK_END support and replaced the hardcoded 0 with SEEK_SET.

tests/load_tests.cpp — Added tests for:

  • File with data_offsets exceeding file boundary (must throw)
  • File with reversed data_offsets (must throw)
  • Valid file still loads correctly (regression)

Checklist

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@angeloskath angeloskath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. The original PR did not actually pass the test that it was supposed to be made for, so I would advise compiling and running the tests before PRing.

It is now fixed and the tests unified.

@angeloskath angeloskath merged commit 50ae312 into ml-explore:main Apr 15, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants