Skip to content

Conversation

@oskarszoon
Copy link
Contributor

What Changed

Fix critical deserialization bug in go-subtree where buf.Read() calls could return partial reads, causing "unable to read conflicting node X" errors.

Problem

The subtree deserialization code used buf.Read() in multiple places, checking if the returned byte count matched the expected amount:

if n, err := buf.Read(st.ConflictingNodes[i][:]); err != nil || n != 32 {
    return fmt.Errorf("unable to read conflicting node %d: %w", i, err)
}

However, bufio.Reader.Read() is allowed to return fewer bytes than requested even when more data is available. When the internal buffer had only 8 bytes remaining (common at 32KB buffer boundaries), it would return those 8
bytes immediately instead of the requested 32, causing the error check to fail.

This manifested as consistent failures at the same conflicting node index (e.g., 1376) across multiple servers because they all had the same buffer size and file layout.

Solution

Replaced all buf.Read() calls with io.ReadFull(), which guarantees reading the full requested amount or returning an error. Also:

  • Removed the unnecessary ReadBytes() helper function that was a poorly-implemented reinvention of io.ReadFull()
  • Cleaned up commented-out io.ReadFull calls that showed previous developers recognized the issue
  • Removed unused variables

@oskarszoon oskarszoon requested a review from mrz1836 as a code owner November 24, 2025 11:54
@github-actions github-actions bot added bug-P3 Lowest rated bug, affects nearly none or low-impact size/S Small change (11–50 lines) labels Nov 24, 2025
@sonarqubecloud
Copy link

@oskarszoon oskarszoon requested a review from ordishs November 24, 2025 11:56
@oskarszoon oskarszoon merged commit b2a9109 into master Nov 24, 2025
47 checks passed
@github-actions github-actions bot deleted the bugfix/subtree-read-buffer branch November 24, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-P3 Lowest rated bug, affects nearly none or low-impact size/S Small change (11–50 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants