string_view: test the bound before dereferencing in CpuFeatures_memchr - #469
Open
EylonKrause wants to merge 1 commit into
Open
string_view: test the bound before dereferencing in CpuFeatures_memchr#469EylonKrause wants to merge 1 commit into
EylonKrause wants to merge 1 commit into
Conversation
CpuFeatures_memchr loops with `for (size_t i = 0; ptr && ptr[i] != '\0' && i < size; ++i)`.
Because && evaluates left-to-right, ptr[i] (the terminator test) is read before
`i < size`. For a view whose [0,size) bytes contain no '\0' and no match, the
loop reaches i == size and reads ptr[size] -- one byte past the view -- before
the bound test stops it.
This backs CpuFeatures_StringView_IndexOfChar/IndexOf/HasWord, which parse
/proc/cpuinfo and FreeBSD /var/run/dmesg.boot via StackLineReader. A newline-free
line >= STACK_LINE_READER_BUFFER_SIZE (1024) fills the reader's `char buffer[1024]`
and calls IndexOfChar(view{buffer,1024}, '\n'), reading buffer[1024] -- past the
array member; for any StringView over a heap slice sized exactly to its content
it is a true out-of-allocation over-read (AddressSanitizer-confirmed).
Reorder the condition so the bound gates the dereference:
for (size_t i = 0; ptr && i < size && ptr[i] != '\0'; ++i)
Behavior is identical for every in-bounds index.
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.
Problem
CpuFeatures_memchr(src/string_view.c) loops with:&&evaluates left-to-right, soptr[i](the terminator test) is read beforei < size. For a view whose[0, size)bytes contain no'\0'and no matching byte, the loop reachesi == sizeand readsptr[size]— one byte past the view — before the bound test stops it.This is the workhorse behind
CpuFeatures_StringView_IndexOfChar/IndexOf/HasWord, which parse/proc/cpuinfoand FreeBSD/var/run/dmesg.bootthroughStackLineReader. A newline-free line>= STACK_LINE_READER_BUFFER_SIZE(1024) fills the reader'schar buffer[1024]and callsIndexOfChar(view{buffer,1024}, '\n'), which readsbuffer[1024]— past the array member. For anyStringViewbacked by a heap slice sized exactly to its content it is a true out-of-allocation over-read.Fix
Reorder the condition so the bound gates the dereference:
Short-circuit
&&makesi < sizegateptr[i]; behavior is identical for every in-bounds index (still stops at the first'\0'or match, still bounded bysize).Testing
malloc(1024)filled with'a',StringView{ptr,1024},IndexOfChar(view,'\n')):AddressSanitizer: heap-buffer-overflow READ of size 1, 0 bytes after 1024-byte regioninCpuFeatures_memchr.-1, no ASan error.string_view_test: 17/17 pass.Disclosure: this contribution was authored with an AI coding assistant (Claude) and reviewed before submission.