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

Socket: delete unix local endpoint filename on Close #52103

Merged
merged 13 commits into from
May 31, 2021
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ public async Task NamedPipeClient_Connects_With_UnixDomainSocketEndPointServer()
}
}

Assert.True(File.Exists(pipeName));
try { File.Delete(pipeName); } catch { }
Assert.False(File.Exists(pipeName));
}
}
}
95 changes: 56 additions & 39 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ public partial class Socket : IDisposable

private SafeSocketHandle _handle;

// _rightEndPoint is null if the socket has not been bound. Otherwise, it is any EndPoint of the
// correct type (IPEndPoint, etc).
// _rightEndPoint is null if the socket has not been bound. Otherwise, it is an EndPoint of the
// correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
// it when the value is still null.
// This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
// and to delete that file when the Socket gets disposed.
Copy link
Member Author

Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPoint UnixDomainSocketEndPoint.BoundFileName.
On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.

Copy link
Member

Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?

Copy link
Member

Choose a reason for hiding this comment

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

I keep asking this question every time I see that variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really know either. Would love to understand.

It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?

internal EndPoint? _rightEndPoint;
internal EndPoint? _remoteEndPoint;

Expand Down Expand Up @@ -293,7 +296,7 @@ public int Available
{
// Update the state if we've become connected after a non-blocking connect.
_isConnected = true;
_rightEndPoint = _nonBlockingConnectRightEndPoint;
_rightEndPoint ??= _nonBlockingConnectRightEndPoint;
Copy link
Member

Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.

It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.
We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.

Copy link
Member

@antonfirsov antonfirsov May 25, 2021

Choose a reason for hiding this comment

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

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.

UpdateLocalEndPointOnConnect();
_nonBlockingConnectInProgress = false;
}
Expand Down Expand Up @@ -340,7 +343,7 @@ public int Available
{
// Update the state if we've become connected after a non-blocking connect.
_isConnected = true;
_rightEndPoint = _nonBlockingConnectRightEndPoint;
_rightEndPoint ??= _nonBlockingConnectRightEndPoint;
UpdateLocalEndPointOnConnect();
_nonBlockingConnectInProgress = false;
}
Expand Down Expand Up @@ -445,7 +448,7 @@ public bool Connected
{
// Update the state if we've become connected after a non-blocking connect.
_isConnected = true;
_rightEndPoint = _nonBlockingConnectRightEndPoint;
_rightEndPoint ??= _nonBlockingConnectRightEndPoint;
UpdateLocalEndPointOnConnect();
_nonBlockingConnectInProgress = false;
}
Expand Down Expand Up @@ -805,11 +808,16 @@ private void DoBind(EndPoint endPointSnapshot, Internals.SocketAddress socketAdd
UpdateStatusAfterSocketErrorAndThrowException(errorCode);
}

if (_rightEndPoint == null)
if (!IsWildcardEndPoint(_rightEndPoint))
{
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint = endPointSnapshot;
_localEndPoint = _rightEndPoint;
tmds marked this conversation as resolved.
Show resolved Hide resolved
}

// Save a copy of the EndPoint so we can use it for Create().
// For UnixDomainSocketEndPoint, track the file to delete on Dispose.
_rightEndPoint = endPointSnapshot is UnixDomainSocketEndPoint unixEndPoint ?
unixEndPoint.CreateBoundEndPoint() :
endPointSnapshot;
}

// Establishes a connection to a remote system.
Expand Down Expand Up @@ -1363,11 +1371,8 @@ public int SendTo(byte[] buffer, int offset, int size, SocketFlags socketFlags,
if (SocketType == SocketType.Dgram) SocketsTelemetry.Log.DatagramSent();
}

if (_rightEndPoint == null)
{
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint = remoteEP;
}
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint ??= remoteEP;

if (NetEventSource.Log.IsEnabled()) NetEventSource.DumpBuffer(this, buffer, offset, size);
return bytesTransferred;
Expand Down Expand Up @@ -1584,11 +1589,8 @@ public int ReceiveMessageFrom(byte[] buffer, int offset, int size, ref SocketFla
catch
{
}
if (_rightEndPoint == null)
{
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint = endPointSnapshot;
}
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint ??= endPointSnapshot;
}

if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, errorCode);
Expand Down Expand Up @@ -1678,11 +1680,8 @@ public int ReceiveMessageFrom(Span<byte> buffer, ref SocketFlags socketFlags, re
catch
{
}
if (_rightEndPoint == null)
{
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint = endPointSnapshot;
}
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint ??= endPointSnapshot;
}

if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, errorCode);
Expand Down Expand Up @@ -1741,11 +1740,8 @@ public int ReceiveFrom(byte[] buffer, int offset, int size, SocketFlags socketFl
catch
{
}
if (_rightEndPoint == null)
{
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint = endPointSnapshot;
}
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint ??= endPointSnapshot;
}

if (socketException != null)
Expand Down Expand Up @@ -3128,10 +3124,7 @@ private bool SendToAsync(SocketAsyncEventArgs e, CancellationToken cancellationT
e.StartOperationCommon(this, SocketAsyncOperation.SendTo);

EndPoint? oldEndPoint = _rightEndPoint;
if (_rightEndPoint == null)
{
_rightEndPoint = endPointSnapshot;
}
_rightEndPoint ??= endPointSnapshot;

SocketError socketError;
try
Expand All @@ -3140,7 +3133,7 @@ private bool SendToAsync(SocketAsyncEventArgs e, CancellationToken cancellationT
}
catch
{
_rightEndPoint = null;
_rightEndPoint = oldEndPoint;
tmds marked this conversation as resolved.
Show resolved Hide resolved
_localEndPoint = null;
// Clear in-use flag on event args object.
e.Complete();
Expand Down Expand Up @@ -3248,11 +3241,8 @@ private void DoConnect(EndPoint endPointSnapshot, Internals.SocketAddress socket

if (SocketsTelemetry.Log.IsEnabled()) SocketsTelemetry.Log.AfterConnect(SocketError.Success);

if (_rightEndPoint == null)
{
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint = endPointSnapshot;
}
// Save a copy of the EndPoint so we can use it for Create().
_rightEndPoint ??= endPointSnapshot;

if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"connection to:{endPointSnapshot}");

Expand Down Expand Up @@ -3380,6 +3370,18 @@ protected virtual void Dispose(bool disposing)
{
}
}

// Delete file of bound UnixDomainSocketEndPoint.
if (_rightEndPoint is UnixDomainSocketEndPoint unixEndPoint &&
unixEndPoint.BoundFileName is not null)
{
try
{
File.Delete(unixEndPoint.BoundFileName);
}
catch
{ }
}
}

// Clean up any cached data
Expand Down Expand Up @@ -3634,9 +3636,24 @@ internal Socket UpdateAcceptSocket(Socket socket, EndPoint remoteEP)
socket._addressFamily = _addressFamily;
socket._socketType = _socketType;
socket._protocolType = _protocolType;
socket._rightEndPoint = _rightEndPoint;
socket._remoteEndPoint = remoteEP;

// If the _rightEndpoint tracks a UnixDomainSocketEndPoint to delete,
// create a new EndPoint.
if (_localEndPoint != null)
{
socket._rightEndPoint = _localEndPoint;
}
else if (_rightEndPoint is UnixDomainSocketEndPoint unixEndPoint &&
unixEndPoint.BoundFileName is not null)
{
socket._rightEndPoint = unixEndPoint.CreateUnboundEndPoint();
}
else
{
socket._rightEndPoint = _rightEndPoint;
}

// If the listener socket was bound to a wildcard address, then the `accept` system call
// will assign a specific address to the accept socket's local endpoint instead of a
// wildcard address. In that case we should not copy listener's wildcard local endpoint.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Diagnostics;
using System.Text;
using System.IO;

namespace System.Net.Sockets
{
Expand All @@ -14,13 +15,22 @@ public sealed partial class UnixDomainSocketEndPoint : EndPoint
private readonly string _path;
private readonly byte[] _encodedPath;

// Tracks the file Socket should delete on Dispose.
internal string? BoundFileName { get; }

public UnixDomainSocketEndPoint(string path)
: this(path, null)
{ }

private UnixDomainSocketEndPoint(string path, string? boundFileName)
{
if (path == null)
{
throw new ArgumentNullException(nameof(path));
}

BoundFileName = boundFileName;

// Pathname socket addresses should be null-terminated.
// Linux abstract socket addresses start with a zero byte, they must not be null-terminated.
bool isAbstract = IsAbstract(path);
Expand Down Expand Up @@ -120,6 +130,24 @@ public override string ToString()
}
}

internal UnixDomainSocketEndPoint CreateBoundEndPoint()
{
if (IsAbstract(_path))
{
return this;
}
return new UnixDomainSocketEndPoint(_path, Path.GetFullPath(_path));
tmds marked this conversation as resolved.
Show resolved Hide resolved
}

internal UnixDomainSocketEndPoint CreateUnboundEndPoint()
{
if (IsAbstract(_path) || BoundFileName is null)
{
return this;
}
return new UnixDomainSocketEndPoint(_path, null);
}

private static bool IsAbstract(string path) => path.Length > 0 && path[0] == '\0';

private static bool IsAbstract(byte[] encodedPath) => encodedPath.Length > 0 && encodedPath[0] == 0;
Expand Down
Loading