Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use io.ReadFull instead of stream.Read #15089

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gazzua
Copy link
Contributor

@gazzua gazzua commented Mar 24, 2025

What type of PR is this?

Bug fix


What does this PR do? Why is it needed?

This PR fixes an issue in readContextFromStream where stream.Read could result in a partial read of the expected forkDigestLength bytes. When reading from a network stream, stream.Read does not guarantee returning all requested bytes in one call, leading to potential truncation or corruption of the context data (e.g., encountering unexpected 0x00 bytes seemed to prematurely "split" the data).

By replacing stream.Read with io.ReadFull, we ensure that:

  • The full forkDigestLength bytes are read correctly.
  • If the stream does not provide enough data, the function returns an error rather than silently returning incomplete data.

This change prevents any unexpected partial reads and ensures the fork digest is always read in full.


Which issues(s) does this PR fix?


Other notes for review

  • The change is minimal but crucial for ensuring data integrity from the stream.

Acknowledgements

@gazzua gazzua requested a review from a team as a code owner March 24, 2025 07:16
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

LGTM - This is a nice convenience wrapper too. Thanks!

Should we do this in all places of stream.Read?

    beacon-chain/sync/error.go|35 col 12| _, err := stream.Read(b)                                                                                                                                                 
    beacon-chain/sync/error.go|82 col 12| _, err := stream.Read(b)                                                                                                                                                 
    beacon-chain/sync/error.go|130 col 13| _, _err := stream.Read([]byte{0})                                                                                                                                       
    beacon-chain/sync/context_test.go|57 col 13| n, err := stream.Read(buf)                                                                                                                                        
    beacon-chain/sync/rpc_goodbye.go|120 col 13| _, _err := stream.Read([]byte{0})                                                                                                                                 
    beacon-chain/sync/rpc_beacon_blocks_by_range_test.go|152 col 14| _, err := stream.Read(b)                                                                                                                      
    beacon-chain/sync/rpc_beacon_blocks_by_range_test.go|271 col 14| _, err := stream.Read(b)

@gazzua
Copy link
Contributor Author

gazzua commented Mar 25, 2025

LGTM - This is a nice convenience wrapper too. Thanks!

Should we do this in all places of stream.Read?

    beacon-chain/sync/error.go|35 col 12| _, err := stream.Read(b)                                                                                                                                                 
    beacon-chain/sync/error.go|82 col 12| _, err := stream.Read(b)                                                                                                                                                 
    beacon-chain/sync/error.go|130 col 13| _, _err := stream.Read([]byte{0})                                                                                                                                       
    beacon-chain/sync/context_test.go|57 col 13| n, err := stream.Read(buf)                                                                                                                                        
    beacon-chain/sync/rpc_goodbye.go|120 col 13| _, _err := stream.Read([]byte{0})                                                                                                                                 
    beacon-chain/sync/rpc_beacon_blocks_by_range_test.go|152 col 14| _, err := stream.Read(b)                                                                                                                      
    beacon-chain/sync/rpc_beacon_blocks_by_range_test.go|271 col 14| _, err := stream.Read(b)

@prestonvanloon
No, we don’t need to apply this change everywhere. The other cases only read 1 byte, so the current implementation is sufficient.

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.

2 participants