Fix stack buffer overflows in h5repack parse_filter (fixes #6433)#6434
Open
brtnfld wants to merge 3 commits into
Open
Fix stack buffer overflows in h5repack parse_filter (fixes #6433)#6434brtnfld wants to merge 3 commits into
brtnfld wants to merge 3 commits into
Conversation
Five sites in parse_filter() wrote to fixed 16-byte stack buffers (scomp[16], stype[16]) via unbounded loop indices with no prior bounds check, causing a stack-buffer-overflow on malformed -f arguments: Site 1: scomp[k] - filter name token before '=' Site 2: stype[m] - SZIP pixels_per_block digit sequence Site 3: stype[m] - SOFF scale_factor digit sequence Site 4: stype[q] - UD legacy per-field digit sequence Site 5: stype[m] - all-other-filters digit sequence Replace all five bare array writes with a single PARSE_BUF_WRITE macro that checks the index against sizeof(buf)-1 before writing, then frees obj_list and exits with an error message on overflow — consistent with all other error handling in parse_filter(). Confirmed with AddressSanitizer: overflow is eliminated and invalid over-long arguments produce a clean diagnostic exit instead of UB.
All five overflow sites in parse_filter() are now protected via a PARSE_BUF_WRITE macro that takes an explicit buf_sz argument (passed as sizeof(buf) at every call site) to avoid silent pointer-decay if the buffers are ever refactored to heap allocations. In the SZIP and SOFF inner loops the previous code wrote bare `smask[l] = c` with no bounds check, and relied on `i = len - 1` to stop the *outer* loop without also breaking the *inner* `u` loop. That left one additional iteration where `l` could equal 2 and `smask[2]` would be written by the next outer iteration before the outer loop condition was re-evaluated. Both sites now use PARSE_BUF_WRITE and add an explicit `break` to exit the inner loop as soon as the two-character mask is complete. The NULL guard around free(obj_list_ptr) is removed: free(NULL) is a safe no-op per C99 §7.20.3.2.
brtnfld
requested review from
bmribler,
jhendersonHDF and
mattjala
as code owners
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
1 participant
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.
Summary
Fixes #6433, three stack buffer overflow vulnerabilities in
parse_filter()intools/src/h5repack/h5repack_parse.c, reported in #6433.scomp[16],stype[16], andsmask[16]parse buffers are now guarded by aPARSE_BUF_WRITEmacro that performs a bounds check before each character write.buf_szargument (passed assizeof(buf)at every call site) to prevent silent pointer-decay if the buffers are ever refactored to heap allocations.uloops, the prior code wrote baresmask[l] = cwith no bounds check and usedi = len - 1to stop the outer loop without breaking the inner loop. A trailing-garbage input such as-f SZIP=16,NNxxxcould therefore write pastsmask[2]. Both sites now usePARSE_BUF_WRITEand add an explicitbreakto exit the inner loop as soon as the two-character mask is complete.if (obj_list_ptr)guard beforefree()is removed;free(NULL)is a safe no-op per C99 §7.20.3.2.