Skip to content

wal: bound record size and tighten repair copy permissions - #694

Open
pfi79 wants to merge 1 commit into
hyperledger:mainfrom
pfi79:wal-record-size-and-perms
Open

pfi79 wants to merge 1 commit into
hyperledger:mainfrom
pfi79:wal-record-size-and-perms

Conversation

@pfi79

@pfi79 pfi79 commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

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 (against FileSizeBytes) and on append. Repair copies are now 0600, and an existing .copy gets chmod'ed, since WriteFile does not touch its mode.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

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

2 open findings
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.

Comment thread pkg/wal/reader.go Outdated
Comment thread pkg/wal/util.go Outdated
@pfi79
pfi79 force-pushed the wal-record-size-and-perms branch 3 times, most recently from fd9c4eb to c180e4a Compare October 8, 2026 19:40
vc.HandleMessage(2, msg2)

reasonAboutInFlightLowerThanLatestDecision.Wait()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason for this change here in this PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

@pfi79
pfi79 force-pushed the wal-record-size-and-perms branch from c180e4a to 7b80b7d Compare October 10, 2026 18:46
Comment thread pkg/wal/writeaheadlog.go
var (
ErrCRC = errors.New("wal: crc verification failed")
ErrWALUnmarshalPayload = errors.New("wal: failed to unmarshal payload")
ErrRecordTooBig = errors.New("wal: record too big")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

Comment thread pkg/wal/reader_test.go
assert.ErrorIs(t, err, ErrRecordTooBig)
assert.Nil(t, record)
assert.NoError(t, r.Close())
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good comment

@pfi79
pfi79 force-pushed the wal-record-size-and-perms branch from 7b80b7d to 44353a7 Compare October 11, 2026 11:15
@pfi79
pfi79 requested review from C0rWin and HagarMeir October 11, 2026 11:20
Comment thread pkg/wal/writeaheadlog.go
errors.Is(readErr, ErrCRC) ||
errors.Is(readErr, ErrWALUnmarshalPayload)) {
errors.Is(readErr, ErrWALUnmarshalPayload) ||
errors.Is(readErr, ErrRecordTooBig)) {

@tock-ibm tock-ibm Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

…ore copying WAL data'

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: pfi79 <pfi79@mail.ru>

wal: bound record size and tighten repair copy permissions

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 (against FileSizeBytes) and on append. Repair copies are now 0600, and
an existing .copy gets chmod'ed, since WriteFile does not touch its mode.

Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
@pfi79
pfi79 force-pushed the wal-record-size-and-perms branch from 44353a7 to fea3f94 Compare October 11, 2026 18:55
@pfi79
pfi79 requested a review from tock-ibm October 11, 2026 18:56
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.

5 participants