wal: bound record size and tighten repair copy permissions by pfi79 · Pull Request #694 · hyperledger/SmartBFT

pfi79 · 2026-10-08T14:19:43Z

A corrupt or forged record header could claim 0xFFFFFFFF bytes and drive a
~4GB allocation on Open. Reject such records with ErrRecordTooBig, both on
read and on append. Repair copies are now 0600, and an existing .copy gets
chmod'ed, since WriteFile does not touch its mode.

Repair no longer performs lossy repairs. Repair() now truncates the
last file only when the corruption is at the tail (the last record is torn).
When a corrupt record is followed by readable records, the repair is refused
with ErrNotRepairable and the file is left untouched (a .copy is still
saved). Previously Repair() silently truncated the file at the first bad
record, which could drop records that were already synced and possibly
committed. This changes whether a node can restart on its own: a mid-file
corruption is no longer repaired automatically.

Torn-tail detection. isTornTail now decides "torn" using the same rules
as LogRecordReader.Read: it accepts a chain as soon as one CRC link is
verified (instead of requiring a clean chain to EOF), probes the exact offset
of the next record first when the bad header is intact, and reseeds the chain
on a CRC_ANCHOR. This makes the repair correct for the usual crash state -
a corrupt record in the middle plus a torn tail.

Record-size bound. readHeader bounds a record against the bytes left in
the file (fileSizeBytes - pos), so a corrupt header near the end of a 64MB
file is rejected before the payload allocation. A header that cannot fit the
file at all is reported as ErrRecordTooBig; a record that only outgrows the
remaining bytes is still io.ErrUnexpectedEOF (a torn tail, the repairable
error). The redundant check in readPayload is removed.

Copilot

🟡 Changes recommended

Repair can truncate valid large records, and pre-existing copy files briefly expose WAL data before permissions are tightened.

2 open findings

Preserve configured WAL size limit during repair · New
Set restrictive file mode before copying WAL data · New

What changed in this PR

Bounds WAL record sizes to prevent oversized allocations and secures repair-copy permissions.

Changes:

Adds ErrRecordTooBig and enforces record-size limits during reads and appends.
Makes repair copies owner-only and adds regression tests.

File	Description
`pkg/wal/writeaheadlog.go`	Enforces configured record limits.
`pkg/wal/reader.go`	Adds bounded record reading.
`pkg/wal/reader_test.go`	Tests oversized records.
`pkg/wal/util.go`	Tightens repair-copy permissions.
`pkg/wal/util_test.go`	Tests private repair copies.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

HagarMeir · 2026-10-10T18:39:38Z

 	vc.HandleMessage(2, msg2)

 	reasonAboutInFlightLowerThanLatestDecision.Wait()
-


no reason for this change here in this PR

C0rWin · 2026-10-10T21:49:12Z

 var (
 	ErrCRC                 = errors.New("wal: crc verification failed")
 	ErrWALUnmarshalPayload = errors.New("wal: failed to unmarshal payload")
+	ErrRecordTooBig        = errors.New("wal: record too big")


ErrRecordTooBig on the last file should go through repair like the other torn-write errors — ReadAll (lines 576-578) doesn't list it, so what used to surface as io.ErrUnexpectedEOF (short ReadFull) and auto-repair now fails startup. scanRepairFile already truncates on any non-EOF error, it just needs to be reachable.

if index == lastIndex && (errors.Is(readErr, io.ErrUnexpectedEOF) || errors.Is(readErr, ErrCRC) || errors.Is(readErr, ErrWALUnmarshalPayload) || errors.Is(readErr, ErrRecordTooBig)) {

C0rWin · 2026-10-10T21:49:12Z

+		assert.ErrorIs(t, err, ErrRecordTooBig)
+		assert.Nil(t, record)
+		assert.NoError(t, r.Close())
+	})


Worth a case in TestWriteAheadLogFile_Repair as well — assertTestRepair only does assert.Error on the first ReadAll, so it can't tell a repairable error from a fatal one. This fails on the current branch and passes with the ReadAll change:

t.Run("oversized record header", func(t *testing.T) { dirPath := filepath.Join(testDir, "oversized") assert.NoError(t, os.MkdirAll(dirPath, walDirPermPrivateRWX)) wal, err := Create(logger, dirPath, &Options{FileSizeBytes: 10 * 1024, BufferSizeBytes: 2048}) assert.NoError(t, err) const NumBytes = 1024 const NumRec = 102 data1 := make([]byte, NumBytes) for m := range NumRec { for n := range NumBytes { data1[n] = byte(m) } assert.NoError(t, wal.Append(data1, false)) } assert.NoError(t, wal.Close()) // append a header claiming a ~4GB record to the last file names, err := dirReadWalNames(dirPath) assert.NoError(t, err) lastFile := filepath.Join(dirPath, names[len(names)-1]) f, err := os.OpenFile(lastFile, os.O_RDWR, walFilePermPrivateRW) assert.NoError(t, err) _, err = f.Seek(0, io.SeekEnd) assert.NoError(t, err) header := make([]byte, recordHeaderSize) binary.LittleEndian.PutUint64(header, 0xFFFFFFFF) _, err = f.Write(header) assert.NoError(t, err) assert.NoError(t, f.Close()) wal, err = Open(logger, dirPath, &Options{FileSizeBytes: 10 * 1024, BufferSizeBytes: 2048}) assert.NoError(t, err) _, err = wal.ReadAll() assert.ErrorIs(t, err, io.ErrUnexpectedEOF) assert.NoError(t, wal.Close()) assertTestRepair(t, logger, dirPath, NumRec) })

Good comment

tock-ibm · 2026-10-11T15:47:05Z

 				errors.Is(readErr, ErrCRC) ||
-				errors.Is(readErr, ErrWALUnmarshalPayload)) {
+				errors.Is(readErr, ErrWALUnmarshalPayload) ||
+				errors.Is(readErr, ErrRecordTooBig)) {


Note that trying to repair on any of these errors may lead the application (fabric) to start smartbft in a state from the past even before a committed record (block). For example, if the file is corrupt somewhere before a TruncateTo(true) control record, you may succeed to "repair" the file, but the end result will be you start smartbft with a wal record that was already committed.

The repair mechanism is against a torn write on the last record, repairing any other corruption is not so easy. We assume that the error is on the last record only. Any other corruption is not necessarily repairable.

Take a look at the test cases in TestWriteAheadLogFile_Repair - only on the tail.

The safe option is to throw away the node and start a new one.

I suggest checking and fixing it only if the error is in the tail. Throw out the rest.

pfi79 · 2026-10-19T07:44:42Z

@HagarMeir @C0rWin @tock-ibm I've made all the corrections. If there are any more, please write. If there is any doubt that this pr is needed at all, also, please write.

tock-ibm · 2026-10-19T13:07:56Z

@HagarMeir @C0rWin @tock-ibm I've made all the corrections. If there are any more, please write. If there is any doubt that this pr is needed at all, also, please write.

I will review again, thanks @pfi79

tock-ibm

Thanks for the updates, @pfi79. The size bound, the ErrRecordTooBig → repair path and the private .copy all look good, and limiting repair to the tail is the right direction. I re-reviewed 8b41335 with the analysis below in mind. Most of it is about whether isTornTail really separates a torn last record from corruption in the middle of the file.

Blocking: a lossy repair is still possible

readableRecordsFollow only reports "records follow" if the chain is clean all the way to EOF. After a crash, the last record is usually torn. So a corrupt record in the middle plus a torn tail (the likely combination), or a second corruption further on, makes every probe return false. isTornTail then returns true, and the file is truncated at the first bad record. This is the case I raised in the earlier thread: starting smartbft from a state before a committed record.

Reproduced on this branch: 20 records of 1KB, flip one CRC bit of R1, truncate the last 100 bytes (tearing R19). Repair() returns nil and the file shrinks from 20716 to 1056 bytes. R2..R18, which were synced and possibly committed, are silently dropped.

Repro test

func TestRepairMidCorruptPlusTornTail(t *testing.T) {
	basicLog, _ := zap.NewDevelopment()
	logger := basicLog.Sugar()
	dir := t.TempDir()
	w, err := Create(logger, dir, &Options{FileSizeBytes: 1024 * 1024})
	assert.NoError(t, err)
	for range 20 {
		assert.NoError(t, w.Append(make([]byte, 1024), false))
	}
	assert.NoError(t, w.Close())
	names, _ := dirReadWalNames(dir)
	last := filepath.Join(dir, names[0])

	r, err := NewLogRecordReader(logger, last)
	assert.NoError(t, err)
	_, err = r.Read() // R0
	assert.NoError(t, err)
	off, _ := r.logFile.Seek(0, io.SeekCurrent)
	assert.NoError(t, r.Close())

	f, err := os.OpenFile(last, os.O_RDWR, walFilePermPrivateRW)
	assert.NoError(t, err)
	b := make([]byte, 1)
	_, _ = f.ReadAt(b, off+5)
	b[0] ^= 1
	_, _ = f.WriteAt(b, off+5) // corrupt R1's CRC
	fi, _ := f.Stat()
	assert.NoError(t, f.Truncate(fi.Size()-100)) // tear R19
	assert.NoError(t, f.Close())

	assert.ErrorIs(t, Repair(logger, dir), ErrNotRepairable) // fails: nil, file truncated to 1056 bytes
}

Suggested fix (details inline):

Accept as soon as one CRC link is verified, i.e. a record whose CRC chains from the previous record's header. Don't require a clean run to EOF.
Set haveCRC only after verifyCRC succeeds.
When the bad record's header is intact, look first at offset + 8 + len + pad, the exact position of the next record, before falling back to probing every offset.
Add the test above.

Other torn-tail cases that are still not handled

These were reproduced on this branch. They are not regressions, but they belong to the same story. Either fix them here or say in the description that they're out of scope:

A full header with 0 payload bytes. io.ReadFull returns io.EOF, not ErrUnexpectedEOF, so ReadAll treats it as clean and the leftover header is never repaired. readPayload could turn that io.EOF into io.ErrUnexpectedEOF.
A garbage tail that decodes to an unknown type (e.g. Type=7). wal: unexpected LogRecord_Type isn't on ReadAll's repairable list, so startup fails.
A torn CRC anchor in a new last file (crash during saveCRC). Open() → checkWalFiles returns io.ErrUnexpectedEOF, but InitializeAndReadAll only calls Repair for errors from ReadAll() (writeaheadlog.go:785), so startup fails.

(A zero-filled tail is recovered correctly.)

Smaller items

copyFile doesn't Sync() the copy before the original is truncated or deleted (inline).
Probing every 8-byte offset and re-walking the chain each time can be O(n²) reads and allocations on a 64MB file (inline).
The bound in readHeader uses the whole file size, not the bytes left after the current position (inline).
An older bug in a touched function: in the loop of scanRepairFile, a failing Seek (util.go:334) returns without r.Close(). On Windows the leaked handle then blocks the truncate.
The Repair() doc comment (writeaheadlog.go:286) still says "we use pre-allocated / recycled files". Files are created fresh with O_CREATE and truncated on close, so this is no longer true, and it hides the fact that a torn write is the only expected source of a bad tail.
The PR description mentions only the size bound and the permissions. Please also describe the mid-file / ErrNotRepairable behavior change, since it changes when a node can restart on its own.

tock-ibm · 2026-10-19T14:40:30Z

+
+		if payloadLength == 0 || pos+int64(recordHeaderSize)+payloadLength > fileSize {
+			// Torn or out of bounds, the chain ends here.
+			return false, nil


This ends the chain as "not following" when the probe reaches a torn or out-of-bounds record, even if valid chained records were already verified before it. Combined with a torn last record (the usual state after a crash), every probe fails and isTornTail returns true, so a mid-file corruption gets truncated (see the repro in the review body). Once at least one CRC link has been verified, this should return true, not false.

tock-ibm · 2026-10-19T14:40:30Z

+
+		if haveCRC && !verifyCRC(prevCRC, crc, payload) {
+			// The chain is broken: from here on there is nothing we can trust.
+			return false, nil


Same issue: a CRC break later in the chain throws away the links already verified before it. A second corruption anywhere after the first makes the repair lossy. Suggest returning true once one link has been verified, and only returning false here when no link has been verified yet.

tock-ibm · 2026-10-19T14:40:30Z

+		}
+
+		prevCRC = crc
+		haveCRC = true


haveCRC is set after the first record, whose CRC can't be checked (it depends on the corrupt record). So a single random record that decodes and ends exactly at EOF counts as "records follow", which contradicts the doc comment above. Suggest setting a verified flag only after verifyCRC succeeds, and returning that flag.

tock-ibm · 2026-10-19T14:40:30Z

+		switch record.Type {
+		case protos.LogRecord_ENTRY, protos.LogRecord_CONTROL, protos.LogRecord_CRC_ANCHOR:
+		default:
+			return false, nil


Nit: CRC_ANCHOR is accepted in the middle of the chain and CRC-checked as if it were a data record. In the real reader an anchor appears only first in a file, and its header CRC is the previous CRC, not a CRC over its payload. This is one example of readableRecordsFollow drifting from LogRecordReader.Read. Could it reuse a reader positioned at pos with a seeded CRC instead of re-implementing the parsing?

tock-ibm · 2026-10-19T14:40:30Z

+
+	fileSize := fileInfo.Size()
+
+	for offset := recordStart + int64(recordHeaderSize); offset+int64(recordHeaderSize) <= fileSize; offset += int64(recordHeaderSize) {


When the corrupt record's header is intact (only its payload or CRC is bad), the next record starts exactly at recordStart + 8 + len + pad. Checking that position first gives a precise answer, and the probing of every offset is then only needed when the length itself is corrupt.

Also on cost: up to fileSize/8 starting offsets, each re-walking the chain with a fresh ReadAt and allocations, is O(n²) in the worst case. On a 64MB file that's ~8M probes at startup. Reading the remainder once into a buffer and scanning in memory would avoid it.

tock-ibm · 2026-10-19T14:40:30Z

+		return err
+	}
+
+	return output.Close()


The copy is the only evidence once scanRepairFile truncates or deletes the original, but it's never fsynced. A power loss right after the repair can leave the original truncated and the .copy empty or partial. Suggest calling output.Sync() before Close().

tock-ibm · 2026-10-19T14:40:30Z


+	// A record, together with its header, must fit in the file it is read from.
+	// Otherwise a corrupted header makes us allocate up to 4GB.
+	if int64(length)+int64(getPadSize(int(length))) > r.fileSizeBytes-int64(recordHeaderSize) {


Minor: this compares against the whole file size, not the bytes left after the current position. A corrupt header near the end of a 64MB file can still declare ~64MB and drive that allocation before ReadFull fails. Bounding by fileSizeBytes - pos would reject it immediately. The check in readPayload (line 188) is then redundant, since every caller passes a length already bounded here.

pfi79 · 2026-10-20T07:24:24Z

Thanks for the updates, @pfi79. The size bound, the ErrRecordTooBig → repair path and the private .copy all look good, and limiting repair to the tail is the right direction. I re-reviewed 8b41335 with the analysis below in mind. Most of it is about whether isTornTail really separates a torn last record from corruption in the middle of the file.

Blocking: a lossy repair is still possible

readableRecordsFollow only reports "records follow" if the chain is clean all the way to EOF. After a crash, the last record is usually torn. So a corrupt record in the middle plus a torn tail (the likely combination), or a second corruption further on, makes every probe return false. isTornTail then returns true, and the file is truncated at the first bad record. This is the case I raised in the earlier thread: starting smartbft from a state before a committed record.

Reproduced on this branch: 20 records of 1KB, flip one CRC bit of R1, truncate the last 100 bytes (tearing R19). Repair() returns nil and the file shrinks from 20716 to 1056 bytes. R2..R18, which were synced and possibly committed, are silently dropped.

Repro test
func TestRepairMidCorruptPlusTornTail(t *testing.T) {
	basicLog, _ := zap.NewDevelopment()
	logger := basicLog.Sugar()
	dir := t.TempDir()
	w, err := Create(logger, dir, &Options{FileSizeBytes: 1024 * 1024})
	assert.NoError(t, err)
	for range 20 {
		assert.NoError(t, w.Append(make([]byte, 1024), false))
	}
	assert.NoError(t, w.Close())
	names, _ := dirReadWalNames(dir)
	last := filepath.Join(dir, names[0])

	r, err := NewLogRecordReader(logger, last)
	assert.NoError(t, err)
	_, err = r.Read() // R0
	assert.NoError(t, err)
	off, _ := r.logFile.Seek(0, io.SeekCurrent)
	assert.NoError(t, r.Close())

	f, err := os.OpenFile(last, os.O_RDWR, walFilePermPrivateRW)
	assert.NoError(t, err)
	b := make([]byte, 1)
	_, _ = f.ReadAt(b, off+5)
	b[0] ^= 1
	_, _ = f.WriteAt(b, off+5) // corrupt R1's CRC
	fi, _ := f.Stat()
	assert.NoError(t, f.Truncate(fi.Size()-100)) // tear R19
	assert.NoError(t, f.Close())

	assert.ErrorIs(t, Repair(logger, dir), ErrNotRepairable) // fails: nil, file truncated to 1056 bytes
}
Suggested fix (details inline):

Accept as soon as one CRC link is verified, i.e. a record whose CRC chains from the previous record's header. Don't require a clean run to EOF.

Set haveCRC only after verifyCRC succeeds.

When the bad record's header is intact, look first at offset + 8 + len + pad, the exact position of the next record, before falling back to probing every offset.

Add the test above.

Other torn-tail cases that are still not handled

These were reproduced on this branch. They are not regressions, but they belong to the same story. Either fix them here or say in the description that they're out of scope:

A full header with 0 payload bytes. io.ReadFull returns io.EOF, not ErrUnexpectedEOF, so ReadAll treats it as clean and the leftover header is never repaired. readPayload could turn that io.EOF into io.ErrUnexpectedEOF.

A garbage tail that decodes to an unknown type (e.g. Type=7). wal: unexpected LogRecord_Type isn't on ReadAll's repairable list, so startup fails.

A torn CRC anchor in a new last file (crash during saveCRC). Open() → checkWalFiles returns io.ErrUnexpectedEOF, but InitializeAndReadAll only calls Repair for errors from ReadAll() (writeaheadlog.go:785), so startup fails.

(A zero-filled tail is recovered correctly.)

Smaller items

copyFile doesn't Sync() the copy before the original is truncated or deleted (inline).

Probing every 8-byte offset and re-walking the chain each time can be O(n²) reads and allocations on a 64MB file (inline).

The bound in readHeader uses the whole file size, not the bytes left after the current position (inline).

An older bug in a touched function: in the loop of scanRepairFile, a failing Seek (util.go:334) returns without r.Close(). On Windows the leaked handle then blocks the truncate.

The Repair() doc comment (writeaheadlog.go:286) still says "we use pre-allocated / recycled files". Files are created fresh with O_CREATE and truncated on close, so this is no longer true, and it hides the fact that a torn write is the only expected source of a bad tail.

The PR description mentions only the size bound and the permissions. Please also describe the mid-file / ErrNotRepairable behavior change, since it changes when a node can restart on its own.

fixed

…ive a ~4GB allocation on Open. Reject such records with ErrRecordTooBig, both on read and on append. Repair copies are now 0600, and an existing .copy gets chmod'ed, since WriteFile does not touch its mode. **Repair no longer performs lossy repairs.** `Repair()` now truncates the last file only when the corruption is at the tail (the last record is torn). When a corrupt record is followed by readable records, the repair is refused with `ErrNotRepairable` and the file is left untouched (a `.copy` is still saved). Previously `Repair()` silently truncated the file at the first bad record, which could drop records that were already synced and possibly committed. This changes whether a node can restart on its own: a mid-file corruption is no longer repaired automatically. **Torn-tail detection.** `isTornTail` now decides "torn" using the same rules as `LogRecordReader.Read`: it accepts a chain as soon as one CRC link is verified (instead of requiring a clean chain to EOF), probes the exact offset of the next record first when the bad header is intact, and reseeds the chain on a `CRC_ANCHOR`. This makes the repair correct for the usual crash state - a corrupt record in the middle plus a torn tail. **Record-size bound.** `readHeader` bounds a record against the bytes left in the file (`fileSizeBytes - pos`), so a corrupt header near the end of a 64MB file is rejected before the payload allocation. A header that cannot fit the file at all is reported as `ErrRecordTooBig`; a record that only outgrows the remaining bytes is still `io.ErrUnexpectedEOF` (a torn tail, the repairable error). The redundant check in `readPayload` is removed. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>

tock-ibm requested a balanced review from Copilot October 8, 2026 18:54

Copilot started reviewing on behalf of tock-ibm October 8, 2026 18:55

View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Comment thread pkg/wal/reader.go Outdated

Comment thread pkg/wal/util.go Outdated

pfi79 force-pushed the wal-record-size-and-perms branch 3 times, most recently from fd9c4eb to c180e4a Compare October 8, 2026 19:40

HagarMeir reviewed Oct 10, 2026

View reviewed changes

pfi79 force-pushed the wal-record-size-and-perms branch from c180e4a to 7b80b7d Compare October 10, 2026 18:46

C0rWin reviewed Oct 10, 2026

View reviewed changes

pfi79 force-pushed the wal-record-size-and-perms branch from 7b80b7d to 44353a7 Compare October 11, 2026 11:15

pfi79 requested review from C0rWin and HagarMeir October 11, 2026 11:20

tock-ibm reviewed Oct 11, 2026

View reviewed changes

pfi79 force-pushed the wal-record-size-and-perms branch from 44353a7 to fea3f94 Compare October 11, 2026 18:55

pfi79 requested a review from tock-ibm October 11, 2026 18:56

pfi79 force-pushed the wal-record-size-and-perms branch 4 times, most recently from b5b0337 to 8b41335 Compare October 19, 2026 07:34

tock-ibm reviewed Oct 19, 2026

View reviewed changes

pfi79 force-pushed the wal-record-size-and-perms branch from 8b41335 to 4a310c4 Compare October 20, 2026 07:23

pfi79 force-pushed the wal-record-size-and-perms branch from 4a310c4 to 9f17f2a Compare October 20, 2026 13:25

		vc.HandleMessage(2, msg2)

		reasonAboutInFlightLowerThanLatestDecision.Wait()


		fileSize := fileInfo.Size()

		for offset := recordStart + int64(recordHeaderSize); offset+int64(recordHeaderSize) <= fileSize; offset += int64(recordHeaderSize) {

Conversation

pfi79 commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tock-ibm Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfi79 commented Oct 19, 2026

Uh oh!

tock-ibm commented Oct 19, 2026

Uh oh!

tock-ibm left a comment

Choose a reason for hiding this comment

Blocking: a lossy repair is still possible

Other torn-tail cases that are still not handled

Smaller items

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfi79 commented Oct 20, 2026

Blocking: a lossy repair is still possible

Other torn-tail cases that are still not handled

Smaller items

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pfi79 commented Oct 8, 2026 •

edited

Loading

tock-ibm Oct 11, 2026 •

edited

Loading