bound nbit decompression reads to the stored chunk length

[naruto-lgtm]

Describe your changes

Originally this bounded the reverse n-bit decompression walk to the stored chunk length. That part has since landed on develop in a3cf1ea (#6497), which is a superset of what was here, so it has been dropped and the branch rebased onto develop. Two things are left:

1. The regression test for the over-read. It passes against develop as it stands, with no changes under src, so it only pins the behaviour a3cf1ea introduced. It stores an 8-byte chunk for a 4000-byte unfiltered chunk via H5Dwrite_chunk() and requires the read to fail rather than decompress out of the uninitialised tail of the chunk buffer.

2. A zero-divisor check in H5Z__nbit_decompress_one_array(). The three divisions there take their divisor from the filter's client-data parameters, and a zero base size still isn't rejected on develop. A zero atomic size also passes the existing precision/offset check when the precision and offset are zero as well, so it isn't caught there either. Confirmed with -fsanitize=integer-divide-by-zero against a crafted file whose stored base size is zeroed: division by zero at H5Znbit.c:1217. On x86-64 that is a SIGFPE; on arm64 UDIV yields 0 instead, so the read quietly returns zeroed data.

The two are separate commits so the test can be taken on its own.

dsets (including test_filter_bad_params) and direct_chunk both pass.

Issue ticket number (GitHub or JIRA)

Follow-up to #6585; the over-read it describes is now fixed by #6497.

Checklist before requesting a review

• My code conforms to the guidelines in CONTRIBUTING.md
• I made an entry in release_docs/CHANGELOG.md (bug fixes, new features)
• I added a test (bug fixes, new features)

[github-actions]

Review Checklist

This PR touches the following areas. Each needs a sign-off
from its listed owners before merging.

• src — @fortnern
• test — @fortnern
• release_docs — @hyoklee

[nopa12]

there is one gap in the same file it doesn't cover: H5Z__nbit_decompress_one_array still divides by a file-controlled size with no zero-check:

n = total_size / p.size;   // also: total_size / base_size

[naruto-lgtm]

Good catch. All three divisions in H5Z__nbit_decompress_one_array take their divisor from the pipeline message, and the precision/offset check passes when both are zero, so a zero size did reach the division. Pushed a zero-check before each one; the nbit tests in dsets and direct_chunk still pass.

[hyoklee]

nbit -> n-bit

Fix grammar:

The reverse n-bit filter walked its bit reader through the chunk buffer using only the element count and precision from the filter pipeline message, never checking the number of bytes actually stored for the chunk. Because the chunk buffer provided to the filter is sized to hold the larger unfiltered chunk, a truncated or corrupted chunk would decompress out of the uninitialized remainder of that allocation and return it as dataset data. The decompression path now tracks the stored length and rejects any read past it.

[naruto-lgtm]

Done, took your wording and switched the heading to n-bit as well.

[github-actions]

This pull request has had no activity for 30 days and has been marked stale. Push a commit or comment to keep it open, or it will be flagged for maintainer review.

[naruto-lgtm]

any update?

[fortnern]

This may have been fixed by this commit last week: a3cf1ea
Do you think the changes in that commit are sufficient? Does your test pass with the latest version of the develop branch of HDF5, without your changes in src? If so, we can still merge just the test from this PR.

[naruto-lgtm]

Yes, a3cf1ea covers it, and it's a superset of what I had here: it also validates cd_values up front and bounds the walk through the parameter list, neither of which my version did. My test passes on current develop with no changes under src, so I've rebased onto develop and dropped my H5Znbit.c bound changes. The test is its own commit now, so it can go in on its own.

One item from the earlier review on this PR is still open on develop though. The three divisions in H5Z__nbit_decompress_one_array() take their divisor from the client-data parameters, and a zero base size isn't rejected. A crafted file with the stored base size zeroed trips it at H5Znbit.c:1217, confirmed with -fsanitize=integer-divide-by-zero. On x86-64 that's a SIGFPE; on arm64 UDIV yields 0 instead, so the read quietly returns zeroed data. That's the second commit, kept separate in case you'd rather handle it your own way.

dsets (including test_filter_bad_params) and direct_chunk both pass with it. I have the generator for that crafted file if you want the zero-size case added to gen_bad_filters.c alongside the other three.