fix: reject WAL record sizes that overflow the codec bounds check

[basavaraj-sm05]

The WAL codec checks a record's declared payload size against what is left in the segment before it reads the payload, but it adds the header size to that declared size in uint32. A size field within a header's width of the uint32 max wraps to a small number, so the guard that is meant to catch an oversized record passes instead, and V2 then slices the payload with a high bound that has wrapped below the low bound. The size is used to slice before the CRC is checked, so a single flipped byte is enough, and since RecoverIndex runs on every WAL open the panic lands on the startup path with nothing to recover it. The quieter consequences bothered me more than the crash: GetRecordSize hands back a wrapped record size, which makes readWriteSegment.Truncate compute an end offset below the offset the caller declared safe and zero out committed records, and V1's RecoverIndex can advance by exactly zero and spin. I ran into this while extending TestV2_BreakingPoint_Size, which already corrupts the size field but uses 123123, a value too small to wrap. Doing the arithmetic in uint64 and requiring the whole header to be in range before reading it covers both codec versions, and that second part also closes a case where ReadInt read four bytes behind a guard that only required one to be present. I kept it inside ReadHeaderWithValidation because every read path funnels through it.

[Copilot]

Pull request overview

This PR hardens the WAL codec header/record validation logic to prevent uint32 overflow from bypassing bounds checks, which could otherwise cause panics during WAL recovery and potentially lead to incorrect truncation behavior.

Changes:

• Perform header/record size arithmetic in uint64 to avoid wraparound in both V1 and V2 codecs.
• Require the full header to be in-bounds before reading any header fields.
• Add regression tests covering size-overflow and partial-header scenarios.

Reviewed changes

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

File	Description
oxiad/dataserver/wal/codec/v2.go	Uses uint64-based bounds checks for full-header and record-size validation in V2.
oxiad/dataserver/wal/codec/v2_test.go	Adds V2 regression tests for size overflow and partial-header offsets.
oxiad/dataserver/wal/codec/v1.go	Mirrors the uint64 bounds-check hardening for V1.
oxiad/dataserver/wal/codec/v1_test.go	Adds V1 regression tests for size overflow, partial headers, and RecoverIndex behavior.

Comments suppressed due to low confidence (2)

oxiad/dataserver/wal/codec/v2.go:123

• This check compares the expected record size against the remaining bytes from startFileOffset, but the error message reports the total buf size. That makes the message inaccurate for non-zero offsets.

	// overflow checking
	actualBufSize := bufSize - uint64(startFileOffset)
	if expectSize > actualBufSize {
		return payloadSize, previousCrc, payloadCrc,
			errors.Wrapf(ErrOffsetOutOfBounds, "expected payload size: %d. actual buf size: %d ", expectSize, bufSize)

oxiad/dataserver/wal/codec/v1.go:106

• This check compares the expected record size against the remaining bytes from startFileOffset, but the error message reports the total buf size. That makes the message inaccurate for non-zero offsets.

	expectSize := uint64(payloadSize) + uint64(v.HeaderSize)
	// overflow checking
	actualBufSize := bufSize - uint64(startFileOffset)
	if expectSize > actualBufSize {
		return payloadSize, previousCrc, payloadCrc,
			errors.Wrapf(ErrOffsetOutOfBounds, "expected payload size: %d. actual buf size: %d ", expectSize, bufSize)
	}

---

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

[basavaraj-sm05]

Right, both messages here were carried over from the old check, where the single comparison really was about the payload size. The header guard now says "expected header end offset", and the record-size check below reports the remaining bytes it actually compares against instead of the whole buffer.

[basavaraj-sm05]

Fixed here as well, same wording as V1: the header guard now names the header end offset, and the record-size check on line 119 reports the bytes remaining after startFileOffset rather than the full buffer size.

[mattisonchao]

Hi @basavaraj-sm05

Could you help review the comment from Copilot?

[basavaraj-sm05]

Copilot's comments were valid, both variants of them. The messages were leftovers from before this change, when there was a single check that really was about the payload size. After splitting it in two I hadn't updated the wording, so the header guard printed the header end offset under a payload-size label, and the record-size guard compared against the bytes remaining after startFileOffset but printed the whole buffer size, which would read wrong for any record past the first. Each message now reports the exact values its comparison uses, in both codec versions. Codec and wal tests still pass with -race.

[basavaraj-sm05]

any update?