Skip to content

string_view: test the bound before dereferencing in CpuFeatures_memchr - #469

Open
EylonKrause wants to merge 1 commit into
google:mainfrom
EylonKrause:fix/memchr-bound-before-deref
Open

string_view: test the bound before dereferencing in CpuFeatures_memchr#469
EylonKrause wants to merge 1 commit into
google:mainfrom
EylonKrause:fix/memchr-bound-before-deref

Conversation

@EylonKrause

Copy link
Copy Markdown
Contributor

Problem

CpuFeatures_memchr (src/string_view.c) loops with:

for (size_t i = 0; ptr && ptr[i] != '\0' && i < size; ++i)
  if (ptr[i] == c) return ptr + i;

&& evaluates left-to-right, so ptr[i] (the terminator test) is read before i < size. For a view whose [0, size) bytes contain no '\0' and no matching byte, the loop reaches i == size and reads ptr[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/cpuinfo and FreeBSD /var/run/dmesg.boot through 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'), which reads buffer[1024] — past the array member. For any StringView backed 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:

for (size_t i = 0; ptr && i < size && ptr[i] != '\0'; ++i)

Short-circuit && makes i < size gate ptr[i]; behavior is identical for every in-bounds index (still stops at the first '\0' or match, still bounded by size).

Testing

  • ASan harness (malloc(1024) filled with 'a', StringView{ptr,1024}, IndexOfChar(view,'\n')):
    • Before: AddressSanitizer: heap-buffer-overflow READ of size 1, 0 bytes after 1024-byte region in CpuFeatures_memchr.
    • After: returns -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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant