fix zero byte Send on linux #51473

geoffkizer · 2021-04-19T00:25:12Z

ghost · 2021-04-19T00:25:19Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	geoffkizer
Assignees:	-
Labels:	`area-System.Net.Sockets`
Milestone:	-

stephentoub · 2021-04-19T01:52:32Z

I assume this is related to #51426?

Sure enough:


Error message
System.Net.Sockets.SocketException : Bad address


Stack trace
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.CreateException(SocketError error, Boolean forAsyncThrow) in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 1287
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.SendAsync(Socket socket, CancellationToken cancellationToken) in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 984
   at System.Net.Sockets.Socket.SendAsync(ReadOnlyMemory`1 buffer, SocketFlags socketFlags, CancellationToken cancellationToken) in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 470
   at System.Net.Sockets.Tests.SendReceive`1.<Send_0ByteSend_Memory_Success>d__9[[System.Net.Sockets.Tests.SocketHelperTask, System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].MoveNext() in /_/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive/SendReceive.cs:line 630
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Net.Sockets.Tests.SendReceive`1.<Send_0ByteSend_Memory_Success>d__9[[System.Net.Sockets.Tests.SocketHelperTask, System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].ExecutionContextCallback(Object s) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs:line 286
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs:line 183
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Net.Sockets.Tests.SendReceive`1.<Send_0ByteSend_Memory_Success>d__9[[System.Net.Sockets.Tests.SocketHelperTask, System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].MoveNext(Thread threadPoolThread) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs:line 324
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Net.Sockets.Tests.SendReceive`1.<Send_0ByteSend_Memory_Success>d__9[[System.Net.Sockets.Tests.SocketHelperTask, System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]], System.Net.Sockets.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].MoveNext() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs:line 302
   at System.Threading.Tasks.SynchronizationContextAwaitTaskContinuation.<>c.<.cctor>b__8_0(Object state) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/TaskContinuation.cs:line 370
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs:line 183
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs:line 147
   at System.Object.lambda_method2(Closure , Object , Object ) in System.Private.CoreLib.dll:token 0x0+0x0
   at System.Threading.Tasks.Task.InnerInvoke() in /_/src/libraries/Sys

geoffkizer · 2021-04-19T01:54:17Z

yep

geoffkizer · 2021-04-19T02:28:33Z

Added code to fix the issue. PTAL.

stephentoub · 2021-04-19T02:33:16Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

+                    {
+                        sent = 0;
+                        errno = Interop.Error.SUCCESS;
+                    }


Are there any SocketFlags that this could potentially cause problems for? Wondering if instead of this, we should ensure we still make the syscall so that the OS can do whatever it needs to do, and just substitute in Array.Empty for buffer.

It's possible. Your suggestion would be safer, for sure.

On the other hand, if someone does a 0-byte send today, this change will save a syscall.

On the other other hand, don't do 0-byte sends.

substitute in Array.Empty for buffer

If we did this we'd still need to pin Array.Empty, right? Is that bad?

If we did this we'd still need to pin Array.Empty, right? Is that bad?

For the duration of the synchronous syscall. That's fine.

That said, it doesn't need to be Array.Empty. It could just be a stack address:

byte b = 0; buffer = MemoryMarshal.CreateReadOnlySpan(ref b, 0); SysSend(..., buffer, ...);

I pushed a new fix. PTAL.

geoffkizer · 2021-04-19T02:33:42Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-04-19T02:33:58Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive/SendReceive.cs

geoffkizer · 2021-04-19T03:15:50Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-04-19T03:16:02Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub · 2021-04-19T03:20:11Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

+        // The Linux kernel doesn't like it if we pass a null reference for buffer pointers, even if the length is 0.
+        // Replace any null pointer (e.g. from Memory<byte>.Empty) with a valid pointer.
+        private static ReadOnlySpan<byte> AvoidNullReference(ReadOnlySpan<byte> buffer) =>
+            Unsafe.IsNullRef(ref MemoryMarshal.GetReference(buffer)) ? Array.Empty<byte>() : buffer;


buffer.IsEmpty would work, too. That wouldn't mean it was a null ref, but it wouldn't hurt to sub in Array.Empty for an empty span even if it's not null.

stephentoub · 2021-04-19T03:22:12Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

                    {
-                        sent = SysSend(socket, flags, buffer, ref offset, ref count, socketAddress, socketAddressLen, out errno);
+                        sent = buffers != null ?
+                            SysSend(socket, flags, buffers, ref bufferIndex, ref offset, socketAddress, socketAddressLen, out errno) :


Is it possible the buffers list contains a default ArraySegment, or do we screen those out? Wondering if the same issue might exist there.

We validate the buffer list when you set it (BufferList setter) and throw on null ArraySegments.

antonfirsov

LGTM

antonfirsov · 2021-04-19T11:11:53Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive/SendReceive.cs

+        public async Task Send_0ByteSend_Success()
+        {
+            using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
+            using (Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
+            {
+                listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
+                listener.Listen(1);
+
+                Task<Socket> acceptTask = AcceptAsync(listener);
+                await Task.WhenAll(
+                    acceptTask,
+                    ConnectAsync(client, new IPEndPoint(IPAddress.Loopback, ((IPEndPoint)listener.LocalEndPoint).Port)));
+
+                using (Socket server = await acceptTask)
+                {
+                    for (int i = 0; i < 3; i++)
+                    {
+                        // Zero byte send should be a no-op 
+                        int bytesSent = await SendAsync(client, new ArraySegment<byte>(Array.Empty<byte>()));
+                        Assert.Equal(0, bytesSent);
+
+                        // Socket should still be usable
+                        await SendAsync(client, new byte[] { 99 });
+                        byte[] buffer = new byte[10];
+                        int bytesReceived = await ReceiveAsync(server, buffer);
+                        Assert.Equal(1, bytesReceived);
+                        Assert.Equal(99, buffer[0]);
+                    }
+                }
+            }
+        }


Nit: it might be worth to dedupe the test code by moving it to a helper method, that can be then invoked from the actual test cases:

protected Task<int> RunZeroByteSendTest(Action<Socket> doSendZeroBytes) { ... }

Even SendReceive_SpanSyncForceNonBlocking could do

RunZeroByteSendTest(client => { Task<int> sendTask = client.SendAsync(ReadOnlyMemory<byte>.Empty, SocketFlags.None).AsTask(); Assert.True(sendTask.IsCompleted); return sendTask; });

antonfirsov · 2021-04-19T11:17:12Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive/SendReceive.cs

+
+                using (Socket server = await acceptTask)
+                {
+                    for (int i = 0; i < 3; i++)


Just wondering: what is the behavior that makes it worth running the same test 3 times on the same socket?

Nothing in particular; I was mimicking other tests that do similar stuff. I can remove if you think it's weird.

wfurt · 2021-04-20T16:45:21Z

is there even any benefit of calling OS if the buffer is empty? I can see it for read but for send I'm not really sure.

geoffkizer · 2021-04-20T16:50:51Z

@wfurt we touched on that above. We decided to be safe and keep the syscall, just in case this has any meaning for a particular socket. If the user cares about avoiding the syscall they can do this themselves.

add zero byte send test

4e4863a

ghost added the area-System.Net.Sockets label Apr 19, 2021

stephentoub approved these changes Apr 19, 2021

View reviewed changes

fix zero byte send on linux and update tests

63053d3

geoffkizer changed the title ~~add zero byte send test~~ fix zero byte Send on linux Apr 19, 2021

geoffkizer marked this pull request as ready for review April 19, 2021 02:28

fix test issue

e389f6f

stephentoub reviewed Apr 19, 2021

View reviewed changes

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive/SendReceive.cs Show resolved Hide resolved

revert previous fix and add AvoidNullReference instead

66fde4d

stephentoub approved these changes Apr 19, 2021

View reviewed changes

antonfirsov approved these changes Apr 19, 2021

View reviewed changes

runfoapp bot mentioned this pull request Apr 19, 2021

Symlinks left on disk by Installer runs breaks job log upload on Linux #51502

Closed

geoffkizer merged commit 333a6c7 into dotnet:main Apr 20, 2021

geoffkizer deleted the zerobytesendtest branch April 20, 2021 16:30

antonfirsov mentioned this pull request May 3, 2021

Adds synchronous span APIs for datagram sockets. #51956

Merged

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

fix zero byte Send on linux #51473

fix zero byte Send on linux #51473

Uh oh!

Conversation

geoffkizer commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 19, 2021

Uh oh!

stephentoub commented Apr 19, 2021

Uh oh!

geoffkizer commented Apr 19, 2021

Uh oh!

geoffkizer commented Apr 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Apr 19, 2021

Uh oh!

azure-pipelines bot commented Apr 19, 2021

Uh oh!

Uh oh!

geoffkizer commented Apr 19, 2021

Uh oh!

azure-pipelines bot commented Apr 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt commented Apr 20, 2021

Uh oh!

geoffkizer commented Apr 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

geoffkizer commented Apr 19, 2021 •

edited

Loading