Skip to content

Handle short reads in PosixRandomAccessFile::Read - #1368

Open
akasakariko wants to merge 1 commit into
google:mainfrom
akasakariko:fix/posix-short-pread
Open

akasakariko wants to merge 1 commit into
google:mainfrom
akasakariko:fix/posix-short-pread

Conversation

@akasakariko

Copy link
Copy Markdown

Fixes #1361

Summary

  • PosixRandomAccessFile::Read previously issued a single pread syscall, treating legal positive short reads as complete reads and failing on reads exceeding Linux's 0x7ffff000 per-call limit
  • PosixRandomAccessFile::Read now loops pread calls until the requested buffer is filled, EOF is reached, or an unrecoverable non-EINTR error occurs
  • Advances buffer pointer and offset by accumulated bytes read across iterations
  • Retries automatically on EINTR without advancing buffer offset
  • Returns already-read bytes in the result slice even if a subsequent read call errors
  • Preserves temporary file descriptor closing on all exit paths
  • Adds Linux-only link-time pread wrapping via -Wl,--wrap=pread in static env_posix_test without public API changes or production hooks
  • Adds 18 parameterized regression tests covering permanent and temporary descriptors, short reads, interrupted reads, EOF, zero-length requests, and partial errors

Testing

  • All tests executed locally; no remote GitHub CI run has been triggered yet
  • Regression test reproduces the failure prior to the fix: 10 of 18 test cases failed with short reads returning truncated data ("cde" instead of "cdefghijkl")
  • Regression test passes with fix applied: 18/18 test cases pass
  • Sanitizer stress test passes: 100 shuffled repetitions pass under Clang with ASan, UBSan, and leak detection enabled
  • Full CTest test suites pass across local builds rebased on bb74ef7:
    • Linux GCC Debug static: 3/3 targets pass
    • Linux GCC Release: 3/3 targets pass
    • Linux Clang Debug static with ASan, UBSan, and leak detection: 3/3 targets pass
    • macOS Clang Debug static: 3/3 targets pass
    • macOS Clang Release shared: 2/2 targets pass (env_posix_test excluded upstream in shared builds)
  • Release build and install succeed: benchmark binaries compile and cmake --install completes cleanly
  • Code style and hygiene checks pass: git diff --check and clang-format on changed lines pass with clean diff
  • Compiler warning checks pass: clang++ with -Wall -Wextra -Werror -Wno-unused-parameter -Wthread-safety passes on modified files util/env_posix.cc and util/env_posix_test.cc

Limitations

  • Did not run the original >2 GiB / ~12 GB RAM end-to-end workload because the local test VM has ~2 GiB RAM; deterministic syscall wrapping was used instead to reproduce short reads safely
  • Local testing only; GitHub CI integration has not run
  • No testing performed on Windows or 32-bit platforms
  • Benchmark binaries were compiled only and not executed
  • Optional compression test cases skipped where libraries were absent: one Zstd test on Linux, and both Snappy and Zstd tests on macOS static build
  • Project-wide -Wall -Wextra with -Werror fails on pre-existing warnings in unmodified source files; standard project build flags pass cleanly

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.

PosixRandomAccessFile::Read treats a short pread() as a complete read: blocks > 0x7ffff000 bytes become unreadable once the mmap limit is exhausted

1 participant