Offset edge case bug fix?

[adamsitnik]

While experimenting with #38747 I've stumbled upon this line:

new ReadOnlySpan<byte>(BufferPtr, Offset + Count)

It should rather be:

new ReadOnlySpan<byte>(BufferPtr + Offset, Count)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[GrabYourPitchforks]

Huh, wonder why unit tests didn't catch that. Maybe our tests only ever exercise the case where Offset = 0?

[adamsitnik]

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 Offset is 0 and it's not a problem or they don't verify what we have actually sent.

I'll try to prepare a failing test on Monday and include it in this PR.

It might need backporting?

[GrabYourPitchforks]

NCL team would know more about the backporting situation. Reliability bugs are normally good candidates for backporting.

[ManickaP]

I don't think it's an error according to this dotnet/corefx#22988 (comment)
But @stephentoub will know more.

[stephentoub]

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

return SocketPal.TryCompleteSendTo(context._socket, new ReadOnlySpan<byte>(BufferPtr, Offset + Count), null, ref bufferIndex, ref Offset, ref Count, Flags, SocketAddress, SocketAddressLen, ref BytesTransferred, out ErrorCode);

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

public static bool TryCompleteSendTo(SafeSocketHandle socket, ReadOnlySpan<byte> buffer, IList<ArraySegment<byte>>? buffers, ref int bufferIndex, ref int offset, ref int count, SocketFlags flags, byte[]? socketAddress, int socketAddressLen, ref int bytesSent, out SocketError errorCode)

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.

[GrabYourPitchforks]

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 (ptr, offset + count) does rather seem like a typo.

[stephentoub]

Sure. If it would help, it could be changed to:

int fullLength = Offset + Count;
... new ReadOnlySpan<byte>(BufferPtr, fullLength) ...

or something along those lines.