Skip to content

Use the current file position in TUring::Read and TUring::Write - #154

Open
resetius wants to merge 1 commit into
masterfrom
uring-file-offset
Open

resetius wants to merge 1 commit into
masterfrom
uring-file-offset

Conversation

@resetius

@resetius resetius commented Nov 4, 2026

Copy link
Copy Markdown
Owner

TUring::Read and TUring::Write passed offset 0 to io_uring_prep_read/io_uring_prep_write. For a regular file that means the start of the file, so the uring file handle re-read the first bytes forever and writes overwrote the beginning.

The fix passes offset -1 (use and advance the current file position), which is also valid for pipes and sockets.

Added test_uring_file_offset: writes a temporary file in several chunks, checks the file contents, then reads it back in several chunks. On master it fails (the file holds 5 bytes instead of 20); with the fix the whole suite passes on Linux aarch64 (kernel 7.3, liburing 2.15) with ASan, and on macOS.

Fixes #151

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Unit Test Results (Linux)

180 tests  +1   180 ✅ +1   14s ⏱️ -1s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 0f29002. ± Comparison against base commit cae2aef.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Unit Test Results (MacOS)

146 tests  ±0   146 ✅ ±0   25s ⏱️ +4s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 0f29002. ± Comparison against base commit cae2aef.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Unit Test Results (Windows)

135 tests  ±0   135 ✅ ±0   39s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 0f29002. ± Comparison against base commit cae2aef.

@resetius

resetius commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner Author

Independent review of head 0f29002. Verdict: correct and ready to merge; no P1/P2 findings.

Findings

  • P3, coroio/backends/uring.cpp:50, :58: the commented-out io_uring_prep_read_fixed / io_uring_prep_write_fixed lines still pass offset 0. They are dead code, but if they are ever revived they will reintroduce the same bug; use -1 there too (or drop them).
  • P3, tests/tests.cpp:1324-1352: test_uring_file_offset covers only a regular file. Pipes, sockets and O_APPEND are not in the test (I checked them by hand, see below). A failed assertion leaks fd (same as the neighbouring uring tests, so not a blocker).

Checked

  • -1 for every kind of fd, with a small liburing repro on the aarch64 host (two reads/writes submitted at once, offset -1): pipe, AF_UNIX stream socketpair and /dev/null all completed with the full length and the data arrived in order; a file opened with O_APPEND got both writes appended after the existing bytes (XXabcdefgh). tty not tested, but the kernel treats it as a non-seekable stream like a pipe.
  • Concurrency on one fd: two writes in flight on a regular file at the same time produced abcdefgh (serialized by the kernel through the file position lock), no overlap. With offset 0 they would both have written at the start, so -1 is not a regression, it is strictly better. As with any positional I/O, the order of completion between concurrent operations on one fd is not guaranteed, so callers must not rely on it.
  • Test is meaningful: with only uring.cpp reverted to master on Linux, test_uring_file_offset fails (20 != 5 at tests.cpp:1340); with the head it passes. The file is unlinked right after mkstemp and fd is closed, so no temp file is left behind; ASan/LSan reported nothing on the head. It is registered inside the HAVE_URING block in main.
  • Same cause elsewhere in uring.cpp: nothing else. The other 0 arguments are recv/send flags, the cancel flags, and the ASYNC_CANCEL prep_rw, none of them file offsets.
  • Repo rules: the commit subject is a bare one-liner without a Co-Authored-By trailer; the PR body has no "Generated with" line; no new comments, no map/allocation on a hot path.

Ran

  • Mac (kqueue), Debug: cmake + ninja build, ctest: 4/4 passed.
  • Linux aarch64 (rpi5, kernel 7.3, liburing 2.15), Debug + ASan: build, ctest: 4/4 passed; ut_tests 139 passed including test_uring_file_offset.
  • Same host with uring.cpp from master: 1 failure, test_uring_file_offset, as above. Restored the head afterwards.
  • CI (gh pr checks): all green (Linux, macOS, Windows).

@resetius resetius added the agent-reviewed An independent review agent checked this PR; the verdict is in a PR comment label Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-reviewed An independent review agent checked this PR; the verdict is in a PR comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TUring::Read/Write always use file offset 0

1 participant