Replace byte buffer with sector buffer

[ChrisSchinnerl]

While our current implementation is theoretically safe it is quite fragile.

bytes.NewBuffer explicitly states to not reuse its input afterwards which is something we do. So if the buffer's implementation changes or some edge case triggers a realloc of the underlying slice, sector might contain corrupted data without us knowing it.

sectorBuffer is a drop-in replacement that makes this impossible and easier to reason about.

[Copilot]

Pull request overview

This PR replaces the use of bytes.NewBuffer with a custom sectorBuffer implementation in the handleRPCWriteSector function to prevent potential data corruption. The change addresses a fragility issue where bytes.NewBuffer explicitly states not to reuse its input, which the code was doing by passing the same sector array to StoreSector after buffering.

Changes:

• Introduced a custom sectorBuffer type that implements io.Writer for safer sector data streaming
• Replaced bytes.NewBuffer(sector[:0]) with newSectorBuffer(&sector) in handleRPCWriteSector
• Added changeset documentation for the patch-level change

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
rhp/v4/server.go	Replaced bytes.NewBuffer with custom sectorBuffer implementation and added the sectorBuffer type definition with its Write method and constructor
.changeset/use_custom_sectorbuffer_rather_than_bytesbuffer_in_handlerpcwritesector.md	Added changeset documentation for the patch-level change

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[n8mgr]

This is a reasonable defensive change, but it doesn't address the other half of the ownership problem which is significantly more likely to bite us in the ass. We have tests that read a full sector without reallocating. A change to stdlib that broke this code would fail our tests first. As-is this is slightly less fragile while still being very fragile.

Can you take this further and enforce ownership of the backing array too? Otherwise, this seems unnecessary.

[ChrisSchinnerl]

This is a reasonable defensive change, but it doesn't address the other half of the ownership problem which is significantly more likely to bite us in the ass. We have tests that read a full sector without reallocating. A change to stdlib that broke this code would fail our tests first. As-is this is slightly less fragile while still being very fragile.

Can you take this further and enforce ownership of the backing array too? Otherwise, this seems unnecessary.

I pushed a little change. Not sure if that is what you had in mind regarding "ownership" but I just thought about all the ways the Rust borrow checker would hate us for that code and this is what I came up with.

[n8mgr]

That is pretty much exactly what I had in mind lol