-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Offset edge case bug fix? #38762
Offset edge case bug fix? #38762
Conversation
Tagging subscribers to this area: @dotnet/ncl |
Huh, wonder why unit tests didn't catch that. Maybe our tests only ever exercise the case where Offset = 0? |
Most probably our tests use a small input that succeeds with the first try and since I'll try to prepare a failing test on Monday and include it in this PR. It might need backporting? |
NCL team would know more about the backporting situation. Reliability bugs are normally good candidates for backporting. |
I don't think it's an error according to this dotnet/corefx#22988 (comment) |
As @ManickaP suggests, the current version is correct; the new one in the PR is not. This should not be merged. Note that in the full line: runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs Line 433 in 7c891ca
Offset and Count are not just being used in creating the span, they're also being passed individually by ref to the operation. TryCompleteSendTo is written in such a way that it's taking the buffer, the offset, and the count: runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs Line 854 in ab39f23
and it's using the offset and the count to index into the buffer. That means the buffer needs to be the full buffer, not just the portion that's going to be read from. The call then updates the offset and count via the byref, so if it read 5 bytes, it's incrementing offset by 5 and decrementing count by 5. All that means that, since BufferPtr is the beginning of the buffer, we can always reconstruct the original length by adding together Offset and Count, hence the Offset + Count. What distresses me is that the tests passed with this PR. If anything, the thing to fix here is ensuring we have tests in place to cover this case. This type will be used when performing synchronous sends on a socket that previously had asynchronous operations performed on it, and a mistake in the arguments here would manifest as a bug if the send didn't complete in one call due to the kernel buffer being full, so it's certainly harder to test, but not impossible. |
Thanks Steve and Marie for that catch! At minimum, it seems like a code comment is in order here, plus adding extra unit test coverage. As Adam pointed out, the code |
Sure. If it would help, it could be changed to: int fullLength = Offset + Count;
... new ReadOnlySpan<byte>(BufferPtr, fullLength) ... or something along those lines. |
While experimenting with #38747 I've stumbled upon this line:
It should rather be: