Skip to content

fix(joiner): read into len(buffer), not cap(buffer) - #5619

Open
martinconic wants to merge 1 commit into
masterfrom
fix/joiner-readat-cap-vs-len
Open

martinconic wants to merge 1 commit into
masterfrom
fix/joiner-readat-cap-vs-len

Conversation

@martinconic

@martinconic martinconic commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

joiner.ReadAt sized its read from cap(buffer) instead of len(buffer):

readLen := min(int64(cap(buffer)), j.span-off)

Reading into a backing[:10] slice whose capacity is 32768 gives three distinct symptoms:

  • Read returns n=32768 for len(p)=10, violating the io.Reader contract (n <= len(p)).
  • 32,758 bytes are written past len(p), into capacity that belongs to the caller.
  • io.LimitedReader.N goes negative (−32758), so the following read reports EOF and io.CopyN(dst, joiner, 10) delivers 512 bytes.

The last one is what bites in practice. http.ServeContent serves every Range request through io.CopyN, so a ranged download returns a short body while Content-Length and Content-Range still advertise the full range — a silent truncation rather than an error.

This is reachable from the API today. GET /bytes/{ref} and GET /bzz/{ref} pass the joiner to http.ServeContent unwrapped when Swarm-Lookahead-Buffer-Size: 0 is set (pkg/api/bzz.go), so a ranged request on that path loses its tail. Measured against an 18 MB object, every large range stopped at exactly 753,664 bytes (23 × 32 KiB), and 80 of 119 requests came back short:

range=0-780000         want=780001  got=753664
range=1044885-1816753  want=771869  got=753664

It stayed hidden because every other caller passes a slice with len == cap: file.JoinReadAll reads into a full 4096-byte buffer, and langos wraps the joiner in a bufio.Reader that fills b.buf[b.w:]. The default download path therefore masks it entirely, and no existing test exercised a short slice with spare capacity.

The fix is caplen. The recursion in readAtOffset is bounded entirely by readLen, so capping it at the buffer length is sufficient to keep every write inside len(buffer).

TestJoinerReadBufferLength covers all three symptoms with Read, ReadAt and CopyN subtests: it asserts the returned count equals the requested length, and that sentinel bytes past len(p) are untouched. All three subtests reproduce the bug when the fix is reverted.

This is long-standing, not a recent regression: cap dates to #667 ("seekjoiner: fill read buffer, integrate langos"), and #2481 only renamed cap(b) to cap(buffer).

Motivation and Context (Optional)

Found while testing whether a Parquet client can query a file directly over /bytes using HTTP range requests. The bug is not specific to that use case — it affects any consumer that reads into a sub-sliced buffer, which includes http.ServeContent for every ranged download taken with lookahead buffering disabled.

AI Disclosure

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

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