Skip to content

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

Merged
gchatelet merged 1 commit into
google:mainfrom
EylonKrause:fix/memchr-bound-before-deref
Sep 3, 2026
Merged

string_view: test the bound before dereferencing in CpuFeatures_memchr#469
gchatelet merged 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.
@gchatelet gchatelet added the bug Something isn't working label Sep 3, 2026
@gchatelet gchatelet added this to the v0.11 milestone Sep 3, 2026
@gchatelet
gchatelet merged commit 625c54d into google:main Sep 3, 2026
38 of 40 checks passed
@gchatelet

Copy link
Copy Markdown
Collaborator

Thx for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants