Return -1 from the unreachable default case in seek_buff - #4763
Open
afonsojanu wants to merge 1 commit into
Open
Return -1 from the unreachable default case in seek_buff#4763afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
seekable_tests.c's seekBuffWithTotal and zstdseek_decompress.c's ZSTD_seekable_seek_buff both fall into a default: assert(0) case for an unsupported seek origin, but never return afterwards. When built with NDEBUG the assert is a no-op, so the switch falls through and newOffset gets compared and stored uninitialized. GCC and Clang both flag this as a real used-uninitialized warning on -O2/-DNDEBUG builds. The default case isn't reachable through any of the library's own call sites (origin is always SEEK_SET/SEEK_CUR/SEEK_END), so there's no runtime input that exercises the fix; verified instead by compiling zstdseek_decompress.c with clang -Wall -Wextra -DNDEBUG -O2 and confirming the used-uninitialized warning is present before the fix and gone after, then rebuilding and running the full contrib/seekable_format/tests suite (all 5 tests pass) with the fix in place.
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.
Fixes #4756.
ZSTD_seekable_seek_buffincontrib/seekable_format/zstdseek_decompress.c(and its duplicate helperseekBuffWithTotalincontrib/seekable_format/tests/seekable_tests.c) both have aswitch (origin)with adefault: assert(0);branch that falls through without returning. WithNDEBUGdefined,assertis a no-op, so the switch falls through andnewOffsetgets compared/stored uninitialized. Both Clang and GCC flag this as a genuine used-uninitialized warning on-O2 -DNDEBUGbuilds (reported here with MSVC/W4 /O2 /DNDEBUG).Fix: add
return -1;after theassert(0), as suggested in the issue.Verified:
zstdseek_decompress.cwithclang -Wall -Wextra -O2 -DNDEBUG: the-Wsometimes-uninitializedwarning onnewOffsetis present before this change and gone after.libzstd.aand rancontrib/seekable_format/tests(make CFLAGS="-O3 -DNDEBUG" test): all 5seekable_testscases still pass.One honest caveat:
originis always one ofSEEK_SET/SEEK_CUR/SEEK_ENDat every call site in the library today, so thedefaultbranch isn't reachable through any public entry point — there's no runtime input that can exercise this fix, only the compiler's static analysis. The fix is still correct and matches the reporter's own suggested mitigation.