Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space

[Marc-André Lureau]

Hi

On Tue, Feb 21, 2023 at 1:13 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Tue, Feb 21, 2023 at 12:18 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 2/20/23 16:29, Marc-André Lureau wrote:
>> >> 7. A Windows SOCKET is also a HANDLE.  Maybe.  I guess.  Docs are
>> >>     confusing.
>> >>
>> > Kind of, but not really. I think a HANDLE is a kind of void*. You need to
>> > be careful using it appropriately with the right functions. Sometime, a
>> > HANDLE can work with generic functions, like ReadFile, but you should not
>> > use a CloseHandle on SOCKET, or registry key..
>>
>> A Windows SOCKET *is* a file HANDLE except it's always in overlapped
>> mode so Windows provides send()/recv() in case you don't want to deal
>> with overlapped mode.  But you can use it with ReadFile too (Windows API
>> documentation says that is only true sometimes, but Winsock API is 30
>> years old and right now you pretty much always can).
>>
>> However, sockets also has some extra information on the side, so you
>> need to close them with closesocket() and CloseHandle() is not enough.
>
>
> Yeah, the question is "is it safe to call CloseHandle() on a SOCKET, before 
> closesocket()". Testing/error checking seems to say it's okay.. I wouldn't be 
> surprised if internally the CloseHandle() function does something to check if 
> the given handle is a SOCKET and skip it. I wish they would document it..
>
>>
>> The problem is that close() of something opened with _open_osfhandle()
>> *does* do that CloseHandle(), so basically you are closing the handle
>> twice.  IIRC there used to be undocumented functions _alloc_osfhnd() and
>> similar, but they don't exist anymore (even Wine does not have them), so
>> we're stuck; unfortunately this is the reason why QEMU is not already
>> doing something like what you have in this patch.
>>
>> Is this a real bug or is it theoretical?  Do file descriptor and socket
>> spaces overlap in practice?
>>
>
> Yes it likely can, the first SOCKET value starts at 92 in a simple test. It 
> looks like it may depend on the system number of opened sockets.
>
> I think the second big issue is that we have many places where we assume a fd 
> is a fd, and we simply call close() (which would result in CloseHandle, but 
> missing closesocket).
>
> sigh, if the CRT would allow us to steal the handle back..
>

I found an interesting option here, using HANDLE_FLAG_PROTECT_FROM_CLOSE:
https://github.com/ksmurph1/VulkanConfigurator/blob/986992a8b963a6b271785a77d5efd349b6e6ea4f/src/folly/src/net/detail/SocketFileDescriptorMap.cpp#L36


And also an old KB:
https://jeffpar.github.io/kbarchive/kb/185/Q185727/
When _open_osfhandle is used on a socket descriptor, both _close() and
closesocket() should be called before exiting. _close() only closes the file
handle. closesocket() has to be called as well to close the socket descriptor
and clean up the underlying socket object.

I'll work on a v3 of the patch/series with the flag trick.




-- 
Marc-André Lureau