Repository navigation
Conversation
….0 (asdf-formatgh-258) Python asdf 5.3.0 checksums compressed blocks over the stored bytes, so the major <= 5 test made correct files from it fail verification. Gate the workaround on < 5.3.0, and even then check the stored bytes first and fall back to the uncompressed data only on a mismatch. Also stop leaking the asdf_meta_t when the writer is not affected, and handle an asdf_library with no name or version.
asdf_ndarray_size and asdf_ndarray_nbytes wrapped, so a file could pass the data size check with a tiny byte count while the conversion loop wrote the true element count into a buffer sized from the wrapped one. Saturate both at UINT64_MAX instead. check_bounds had the same problem with origin + shape, letting a tile or asdf_ndarray_read_at index read outside the array.
print_node followed aliases transparently, so an alias to an ancestor recursed until the stack overflowed, and a few hundred bytes of nested aliases could expand to billions of lines. Track the containers on the current path and mark a repeat as a cycle. Also remember every container printed: meeting one again means it was reached through an alias, and once MAX_REPEATED_NODES nodes have been printed that way further repeats are collapsed. Ordinary aliases are still expanded in full.
cruzzil
force-pushed
the
fix-ndarray-size-overflow
branch
from
October 18, 2026 14:02
27fc575 to
1e40ea7
Compare
asdf_block_data_set_compressed copied all four bytes of the header field from the caller's string, over-reading any name shorter than three characters.
Apply `asdf_ndarray_t.offset` to the pointer returned by `asdf_ndarray_data` and friends. All tile/per-element reads thus also honor it as a result. For compressed data `asdf_ndarray_data_raw` ignores the (in this case unmeaningful) offset, whereas for non-compressed data it's equivalent to `asdf_ndarray_data`, honoring the existing documented invariant).
As described in asdf-format#273, `asdf_ndarray_data()` still returns the base data array as before, but the `asdf_ndarray_read_*` and `asdf_ndarray_at_` that go through type conversions also handle strides when copying to the destination buffer. This also fixed a few small bugs in the process. Was easier to nail down than I feared: mostly achieved through some minor refactoring of the ndarray read internals. The trickiest part is the bounds checking and I'm not completely confident in it but the tests look good.
Implement basic support for ndarray offset and strides
This also reveals through compiler warnings of unusued functions (which it did not previously do for non-static functions with default linkage), revealing two tests that weren't run (not added to the test suite). One of those tests was not passing under ASan, which revealed a small memory leak. The other works fine (trivial negative case). Weirdly, I already had made this fix on another branch some time ago, but it seems to have gotten lost somewhere...
test: make generated test function stubs static
The internal logging functions such as `asdf_log_file` now go through `asdf_log_emit` which actually builds a log record on stack (`asdf_log_rec_t`) and pass it to the registered handler, or the default handler which is the existing stream (stderr) based logging. While we were at it we impoved the stream logging by bracketing it with f(un)lockfile to prevent interleaved writes. This knowingly introduces a slight ABI breakage due to the change in size of the `asdf_config_t` struct. This is accepted on the basis that no known software is using this yet in production. Additional reserved areas are added to all the public config structs to mitigate this in the future when new config settings are needed. Use of log capture in the tests is also reworked. The old method of replacing the log stream with an open_memstream and capturing the log text is still used in some tests of the logging system itself. But in other tests where we want to capture and test log messages we can simply record the emitted `asdf_log_rec_t` to a memory buffer, which is easier for parsing and checking.
Started using `version-added` directive which was only added in Sphinx 9.0 (previously existed but spelled `versionadded`). I like the new spelling better, and am also running Sphinx 9.1 locally. Better to have CI more closely match what I'm actually building the docs on.
feat: structured logging and custom log handlers asdf-formatgh-279
…overread fix: don't read past the end of a short compression name
fix: asdf info on alias cycles and exponential alias expansion
…orkaround fix: limit the Python asdf checksum workaround to versions before 5.3.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review notes, not the upstream description. Defect 1 in libasdf-rs
docs/UPSTREAM-BUGS.md.Bug:
asdf_ndarray_size/asdf_ndarray_nbyteswrap. Withshape: [2**63 + 1]int16 read as int32, the size check sees 2 bytes,malloc(4), andconvert_int16_to_int32writes 2**63+1 elements. Still present at upstreama76a152: ASan heap-buffer-overflow WRITE atndarray_convert.c:409, throughasdf_ndarray_read_all. Thedata_size < asdf_ndarray_nbytes()check from asdf-format#277 doesn't help, becausenbyteswraps too.Also found:
check_boundscomputesorigin + shape, which wraps. Without the fix, anoriginofUINT64_MAXreturnedASDF_NDARRAY_OKfromasdf_ndarray_read_tile_ndim, an out-of-bounds read. The same goes forasdf_ndarray_read_at.Fix: both size functions saturate at
UINT64_MAX; a zero dimension still gives 0.check_boundscompares without adding. The header docs mention the saturation.Tests:
ndarray_read_size_overflow(new 319-byte fixture) andndarray_read_tile_bounds_overflow. Both fail before the fix and pass after.Checked: CMake Debug + ASan, with MD5, all 27 tests pass with no ASan reports. clang-format 18/19/22 agree on the changed lines. clang-tidy adds one new warning (bool→int in a ternary), a pattern upstream's own code already has. The autotools build was not run;
Makefile.amonly gains the fixture inEXTRA_DIST.