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

Consider exposing Socket.Handle / Socket.SafeHandle #16666

Closed
stephentoub opened this issue Mar 10, 2016 · 33 comments
Closed

Consider exposing Socket.Handle / Socket.SafeHandle #16666

stephentoub opened this issue Mar 10, 2016 · 33 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net
Milestone

Comments

@stephentoub
Copy link
Member

In the full framework, Socket provides an IntPtr Handle { get; } property that returns a representation of the underlying OS' socket. This enables using functionality not exposed via the Socket object model. In corefx, this property isn't available, and there's no IntPtr-based or SafeHandle-based replacement for it, preventing such use.

@davidsh
Copy link
Contributor

davidsh commented Mar 10, 2016

We could add a new extension method (with a different name other than Handle) to the System.Net.Sockets 4.1 contract (which hasn't RTM'd). This extension method would simply return a SafeHandle* of the IntPtr. This would allow it be to consumable on .NET Desktop (and implement-able) via the partial façade.

@davidsh
Copy link
Contributor

davidsh commented Mar 10, 2016

cc: @CIPop

@CIPop
Copy link
Member

CIPop commented Mar 10, 2016

Please add the scenarios that you have in mind for this API.

Would those scenarios require a new constructor Socket(SafeHandle handle) as well?

@stephentoub
Copy link
Member Author

Please add the scenarios that you have in mind for this API. Would those scenarios require a new constructor Socket(SafeHandle handle) as well?

The ones I had in mind, no. On Unix, a file descriptor underlies the Socket, and there are some functions that can take this file descriptor and do things with it not exposed in the Socket object model, for example getting details on who's at the other end of a Unix domain socket (http://linux.die.net/man/3/getpeereid).

@davidsh
Copy link
Contributor

davidsh commented Mar 10, 2016

@stephentoub Feel free to drive this API enhancement into API review and implementation/testing including .NET Framework partial façade changes. Overall, I think it is probably an ok thing to add. But I don't think our team has cycles to work this now given the other work we're stabilizing for RTM.

@tmds
Copy link
Member

tmds commented Mar 18, 2016

Just found this issue while I was trying to figure out how to call sendmsg on a Unix Socket.

@tmds
Copy link
Member

tmds commented Mar 19, 2016

Thinking about this further, even if the Socket would expose a handle it isn't clear how I'd call the system function.
Using "libc" in DllImport won't be a cross-platform solution.

@stephentoub
Copy link
Member Author

Using "libc" in DllImport won't be a cross-platform solution.

You'd need to special-case based on platform, either compiling different assemblies based on the target, or with runtime checking, e.g.

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
    // call Win32 functions via P/Invoke
}
else
{
    // call POSIX functions via P/Invoke
}

@tmds
Copy link
Member

tmds commented Mar 19, 2016

If I DllImport libc to call a POSIX function, will that work on all non-Windows platforms? Or are there (non-Windows) platforms that need a value other than libc?

@stephentoub
Copy link
Member Author

It depends on the function. Most POSIX functions will be exported from libc, but it's possible some won't. This is where https://github.com/dotnet/coreclr/issues/930 and https://github.com/dotnet/coreclr/issues/444 come into play.

@tmds
Copy link
Member

tmds commented Mar 19, 2016

Specifically for sendmsg, which has a signature:

ssize_t sendmsg(int sockfd, const struct msghdr *msg, int flags);

is this the proper way to do the DllImport?

[DllImport ("libc", SetLastError=true)]
public static extern IntPtr sendmsg(int sockfd, IntPtr msg, int flags);

@stephentoub
Copy link
Member Author

Is there a reason you can't use Socket.Send? Some option it's not providing that you need?

is this the proper way to do the DllImport?

That looks right (though I think it's possible, though unlikely, that ssize_t could be 64-bit on a 32-bit platform, in which case IntPtr would end up being the wrong size).

@tmds
Copy link
Member

tmds commented Mar 20, 2016

I want to pass credentials using SCM_CREDS in msghdr.msg_control.

@tmds
Copy link
Member

tmds commented Mar 21, 2016

@SidharthNabar Do you have an idea when you will look at this?

@tmds
Copy link
Member

tmds commented Mar 23, 2016

@stephentoub It looks like this can be implemented as return m_Handle.DangerousGetHandle(). Is there a catch?

@davidsh
Copy link
Contributor

davidsh commented Mar 23, 2016

It looks like this can be implemented as return m_Handle.DangerousGetHandle() . Is there a catch?

While that can be implemented in CoreFx, it will not work for .NET Framework (Desktop). We can't use the "m_Handle" field since it is a private field to the Socket class. The revised package for System.Net.Sockets needs to work on both CoreFx and Desktop. We need to implement this functionality as an extension method most likely so that we can use PUBLIC Socket APIs to implement it.

@tmds
Copy link
Member

tmds commented Mar 23, 2016

@davidsh why not implement it as a property on the Socket class (like it is on the desktop framework)?

@davidsh
Copy link
Contributor

davidsh commented Mar 23, 2016

why not implement it as a property on the Socket class (like it is on the desktop framework)?

Because it is an existing class in the .NET Framework and we can't add properties to it without moving it out of the current contract generation it is in. Otherwise, the package will not be portable anymore to Desktop.

I know this is a complicated topic and I can't really fully explain it here. But reving the contract surface for classes that already exist in the .NET Framework is a complex process. @ericstj can explain more fully.

@tmds
Copy link
Member

tmds commented Mar 23, 2016

The way I look at it, the property already exists in the desktop framework so there is no need to change the contract (https://msdn.microsoft.com/en-us/library/system.net.sockets.socket.handle(v=vs.110).aspx).
I'm interested in understanding better, so @ericstj if you have some time...

@stephentoub
Copy link
Member Author

The concern with exposing the existing IntPtr Handle { get; } is that it's exposing the raw handle, rather than exposing it as a SafeHandle that protects against use-after-free and other such problems. The concern with exposing a new SafeHandle SafeHandle { get; } is that it doesn't exist in the full framework yet.

@tmds
Copy link
Member

tmds commented Mar 23, 2016

If the SafeHandle is exposed, doesn't that expose the IntPtr as well (by calling DangerousGetHandle)?

@stephentoub
Copy link
Member Author

If the SafeHandle is exposed, doesn't that expose the IntPtr as well (by calling DangerousGetHandle)?

Yes, but that's why it's prefixed with "Dangerous", and it also exposes the DangerousAddRef and DangerousRelease methods that enable you to do the right thing if you do need the raw IntPtr. Most of the time you don't, though, in that you can write your P/Invokes to take a SafeHandle and the runtime calls AddRef/GetHandle/Release appropriately rather than you doing it manually.

SafeHandle isn't about making it impossible to shoot yourself in the foot, but rather about making the default behavior as reliable as possible.

@davidsh
Copy link
Contributor

davidsh commented Mar 23, 2016

If the SafeHandle is exposed, doesn't that expose the IntPtr as well (by calling DangerousGetHandle)?

Yes, but we don't want to encourage bad practices. So, new APIs expose SafeHandle even though a developer can drill into it to get an IntPtr.

@tmds
Copy link
Member

tmds commented Mar 23, 2016

What needs to happen to add SafeHandle to the System.Net.Sockets contract?

@tmds
Copy link
Member

tmds commented Mar 23, 2016

@davidsh Perhaps it is also an option to extend the current implementation with IntPtr Handle and create a new contract which deprecates the IntPtr in favor of a SafeHandle?

What is the rationale for choosing between an extension method or a property when updating the contract?

@CIPop
Copy link
Member

CIPop commented Mar 23, 2016

@tmds

What is the rationale for choosing between an extension method or a property when updating the contract?

We want to ensure that System.Net.Sockets is compatible between .Net Desktop.
The constraint is that we cannot add new APIs to released .Net versions (e.g. 4.5.2, 4.6). We can though work around this constraint by adding extension methods within the NuGet package implementation. For an example, see the TPL Async additions to Socket.

@tmds
Copy link
Member

tmds commented Mar 24, 2016

@CIPop Is there an advantage to using an extension method when the new feature cannot be implemented on top of the existing public API?

@tmds
Copy link
Member

tmds commented Mar 24, 2016

@CIPop Let me paraphrase my question:

In the first post on this issue @davidsh said:

We could add a new extension method (with a different name other than Handle) to the System.Net.Sockets 4.1 contract (which hasn't RTM'd).

I understand that adding a new feature requires a new version of the contract. But I don't get why it needs to be an extension method. Can't the contract specify whatever it wants (as long as it is backwards compatible)?

@tmds
Copy link
Member

tmds commented Apr 4, 2016

@stephentoub @CIPop @davidsh can we decide something on the API?
Next to the current feature: retrieve IntPtr/SafeHandle from Socket.
Maybe we can also consider: create Socket from IntPtr/SafeHandle?

@tmds
Copy link
Member

tmds commented Apr 19, 2016

bump
From the response rate, it seems this is a low priority issue.
It would enable many unsupported scenarios. Also it would be nice if this were part of netstandard.

@CIPop CIPop assigned himadrisarkar and unassigned SidharthNabar Apr 19, 2016
@tmds
Copy link
Member

tmds commented May 4, 2016

I've settled on using @stephentoub's reflection hack. It limits portability, but in my specific case that's fine.

If the Handle issue isn't considered for the System.Net.Socket 4.1.0 contract used for netstandard 1.3-5, does that mean that when it gets added in 4.1.1 for netstandard 1.6, access to this feature in a platform generic way will be limited to 1.6+ platforms?

@davidsh
Copy link
Contributor

davidsh commented May 4, 2016

If the Handle issue isn't considered for the System.Net.Socket 4.1.0 contract used for netstandard 1.3-5, does that mean that when it gets added in 4.1.1 for netstandard 1.6

To clarify about contract revisions, System.Net.Sockets 4.1.* is the pending contract which will RTM. When we add new APIs to a contract, we increase the minor version number, so the new contract would be 4.2.* and not 4.1.1.

At this point, it is hard to say whether System.Net.Sockets 4.2.* will have to move to netstandard1.6 or not. That will get investigated post RTM.

@ericeil
Copy link
Contributor

ericeil commented Sep 16, 2016

dotnet/corefx#11800 added Socket.Handle
dotnet/corefx#11807 covers using this from SafePipeHandle.

@ericeil ericeil closed this as completed Sep 16, 2016
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net
Projects
None yet
Development

No branches or pull requests

8 participants