Fix binarySearch() convergence at page boundaries#159
Open
94xhn wants to merge 2 commits into
Open
Conversation
binarySearch() in src/embedDB/embedDB.c fell off the end of a non-void function without returning on its normal termination path (first >= last), which is control-reaches-end-of-non-void-function UB per C11 6.9.1p12 and reproduces with -Wreturn-type. The unconditional pre-check "if (first >= last) break;" exited the loop before the buffered page min/max key was ever compared against the target key, so the function returned an indeterminate value on every normal binary-search convergence, not just a rare edge case. embedDBGet() uses this return value directly to decide whether a record exists, so an existing record could be misreported as missing (false negative) or an out-of-range key falsely reported as found, whenever the caller enables EMBEDDB_USE_BINARY_SEARCH. Fix mirrors the sibling linearSearch() function in the same file: move the "no more room to narrow" exit to after the compareKey check on each branch, and make it an explicit return -1 instead of an unconditional break with no trailing return. Verified with a standalone extraction of the exact function: before the fix, -Wall -Wextra -Wreturn-type reports control-reaches-end-of- non-void-function at -O0 and -O2, and the garbage return value is uncorrelated with actual key membership. After the fix, zero warnings and an 8-case test matrix (single-page hit/miss, multi-page middle/ boundary hits, below-all/above-all misses) all pass at -O0 and -O2.
A converged interval check does not prevent pageId - 1 from underflowing when the midpoint is already the first page. Guard the actual midpoint boundaries and cover the two-page lower-key case with the real file interface. Constraint: preserve the existing binary-search page selection API Confidence: high Scope-risk: narrow Tested: Linux official test 8/8; GCC 13 ASan/UBSan 8/8; MinGW GCC 8 8/8 Not-tested: Arduino targets
94xhn
changed the title
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.
Problem
binarySearch()read the final candidate page and then exited the loop without returning a value. Its first correction still checkedfirst >= last, which is not sufficient when the midpoint is already one boundary of a two-page range: for a key below both pages,pageIdis 0 whilefirst < last, solast = pageId - 1wraps toUINT32_MAXand the search reads an unrelated page.Fix
Always compare the loaded page first. When the key is below it, stop if
pageId == first; when the key is above it, stop ifpageId == last. Otherwise narrow the range as before.A regression test builds two real data pages, queries a key below the minimum, wraps the real file interface to record reads, and verifies that the search returns
-1after reading only page 0. Before the final fix it read page 647.Tests
test_embedDB: 8/8 pass-Werror=return-typecompile check forembedDB.c