boundscheck read replies from the server #39
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
connorkuehl
force-pushed
the
verify_read_reply_offsets
branch
3 times, most recently
from
7991ac1 to
4491db1
Compare
sam-github
approved these changes
Dec 4, 2025
sam-github
left a comment
sam-github
left a comment
There was a problem hiding this comment.
I think span could use some comments, even though the pkg is internal, but otherwise LGTM.
|
|
||
| if int(normalizedOffset)+int(datalen) > len(buf) { | ||
| return n, errors.New("server chunk is too large for given buf") | ||
| if err := got.Check(); err != nil || !wantedBlocks.Contains(got) { |
There was a problem hiding this comment.
I read span first, it looks like this is only usage of Contains(), and the Check() is done here. Forcing all callers to check that spans are valid and making the methods assume they are is a classic optimization, makes sense if documented, but it might be prone to misuse.
Collaborator
Author
There was a problem hiding this comment.
Check() is used in two places where the input to the Span is untrusted. For example, wantedBlocks outside of the loop uses trusted input and inspection:
wantedBlocks := span.Span[uint64]{
Start: offset,
End: offset + uint64(len(buf)),
}
The NBD protocol states: > The data MUST lie within the bounds of the original offset and > length of the client's request If a server did send a block outside of that range, I expect the previous code would wrap around the unsigned integer for the normalizedOffset calculation, which may still accidentally trigger the previous code's error condition, albeit with a somewhat confusing error message. Hopefully these explicit bounds checks provide clearer error messages.
connorkuehl
force-pushed
the
verify_read_reply_offsets
branch
from
4491db1 to
b286f61
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The NBD protocol states:
If a server did send a block outside of that range, I expect the
previous code would wrap around the unsigned integer for the
normalizedOffset calculation, which may still accidentally trigger
the previous code's error condition, albeit with a somewhat confusing
error message.
Hopefully these explicit bounds checks provide clearer error messages.