Skip to content

Handle read errors safely in StackLineReader to prevent buffer overreads - #472

Open
jdymitarai wants to merge 1 commit into
google:mainfrom
jdymitarai:fix-stack-line-reader-read-error
Open

Handle read errors safely in StackLineReader to prevent buffer overreads#472
jdymitarai wants to merge 1 commit into
google:mainfrom
jdymitarai:fix-stack-line-reader-read-error

Conversation

@jdymitarai

Copy link
Copy Markdown

Summary of Changes

  1. Fix integer underflow and buffer overreads on read failures:

    • In LoadFullBuffer and LoadMore, CpuFeatures_ReadFile returns -1 on I/O error or invalid file descriptors.
    • In non-debug / release builds (NDEBUG defined), assert(read >= 0) is omitted.
    • Assigning a negative integer to unsigned size_t reader->view.size (or adding it via += read) caused reader->view.size to underflow to SIZE_MAX (18446744073709551615ULL).
    • Consequently, subsequent operations in IndexOfEol / CpuFeatures_memchr performed unbounded out-of-bounds reads past reader->buffer on the stack, leading to segmentation faults or infinite loops.
  2. Defensive handling:

    • In LoadFullBuffer, if read <= 0, reset reader->view.size = 0 and return -1 (if < 0) or 0.
    • In LoadMore, if read <= 0, return immediately without modifying reader->view.size.
    • In SkipToNextLine and StackLineReader_NextLine, check read <= 0 to treat read errors as EOF cleanly.
  3. Test Infrastructure & Coverage:

    • Extended test/filesystem_for_testing to support simulated read errors and FindFileOrNull(int fd) for invalid descriptors.
    • Added unit tests in test/stack_line_reader_test.cc covering:
      • Invalid file descriptor (-1) initialization and reading.
      • Mid-stream read failure after reading valid lines.
      • Read failure occurring while in line truncation skip mode.

Verification

All tests in stack_line_reader_test, string_view_test, cpuinfo_x86_test, and bit_utils_test pass cleanly on MSVC.

1. Check read <= 0 in LoadFullBuffer and LoadMore to prevent integer underflow and out-of-bounds reads in release builds where assert() is disabled.
2. Gracefully treat read errors as EOF in SkipToNextLine and StackLineReader_NextLine.
3. Support read error simulation in test filesystem harness.
4. Add comprehensive unit tests covering invalid file descriptor, mid-stream read failure, and read errors during line truncation skip mode.
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