Skip to content

Check a container's payload fits the file before allocating for it - #669

Closed
lenamonj wants to merge 1 commit into
ARM-software:mainfrom
lenamonj:loader-payload-check
Closed

lenamonj wants to merge 1 commit into
ARM-software:mainfrom
lenamonj:loader-payload-check

Conversation

@lenamonj

@lenamonj lenamonj commented Sep 8, 2026

Copy link
Copy Markdown

The uncompressed KTX and DDS loaders, the compressed KTX loader and the .astc loader size an allocation from header fields and only discover afterwards that the file does not hold that much data. The allocation is zero-filled and touched, so a 16-byte .astc declaring 65535x65535 reaches 4.2 GB of resident memory and a 128-byte DDS declaring the same reaches 12 GB before the short read fails. (Corrected: an earlier version of this description claimed a compressed KTX could succeed with a truncated buffer; it cannot, every loader checks the read.)

Each site now checks the declared payload against the bytes left in the stream before allocating, and reports a corrupt header. The ASAN caveat on test_dl_corrupt_astc_huge_size goes with it.

Verified with /usr/bin/time -v on the four files (4 to 6 MB peak after) and the x64 CI recipe.

The uncompressed KTX and DDS loaders, the compressed KTX loader and the
.astc loader sized their allocation from header fields and only found out
afterwards that the file did not hold that much data. The allocation is
zero-filled, so a 68-byte KTX declaring 65535x65535 committed 4 GB of
resident memory before failing, and the compressed KTX loader returned
success with a truncated buffer. Every such site now compares the declared
payload against the bytes left in the stream first and reports a corrupt
header instead.
@solidpixel

solidpixel commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Is there any actual bug here? Either:

  • A security issue where we read out-of-bounds?
  • A case where we claimed to succeed but returned incorrect output?

If yes, please provide specific reproducer data files.

a 68-byte compressed KTX returns success with a truncated buffer

This one sounds like it might be a functional bug, but I'm not entirely sure and can't reproduce it locally. It will fail if either reported data length is less than the required size for the declared number of blocks, or if the read fails to read the declared number of bytes, neither of which smells like a bug.

@lenamonj

lenamonj commented Sep 8, 2026

Copy link
Copy Markdown
Author

You are right on the compressed KTX, and I have corrected the description. Every loader checks file.fail() after the payload read, so a truncated file ends in File read failed on main; I could not reproduce a success with a short buffer, and that sentence should not have been there.

What remains is only the allocation itself: the zero-filled buffer is committed and touched before the read fails, so a 16-byte .astc or a 128-byte DDS declaring 65535x65535 reaches 4 GB and 12 GB of resident memory on the way to that error, and the ASAN functional test carries its caveat for the same reason. No out-of-bounds access and no wrong output. If avoiding that transient allocation is not worth a check in the loaders, I am happy for this to be closed.

@solidpixel

Copy link
Copy Markdown
Contributor

Closing - no user-visible bug.

@solidpixel solidpixel closed this Sep 8, 2026
@lenamonj
lenamonj deleted the loader-payload-check branch September 8, 2026 10:06
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