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

LibCore: Consistently treat file descriptors as handles on Windows #3166

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stasoid
Copy link
Contributor

@stasoid stasoid commented Jan 7, 2025

This PR replaces #2946. It is a prerequisite for LibIPC.

Before porting LibIPC we used CRT file descriptors for files/directories/sockets and file mapping handle (cast to int) for AnonymousBuffer.
When porting LibIPC it became apparent that System::dup and System::close need to be able to work with all types of objects mentioned above.
To address this problem I created #2946 which took a safe route: it allowed to coexist CRT fds and handles by using sign bit to distinguish between them.
Also I realized that CRT fds are better not be used for sockets.
I was already on edge about #2946 when I wrote it. The solution seemed a bit too convoluted for the problem it tried to solve.

These two thing pushed me over the edge:

  • All validity checks of the form fd < 0 should be changed to fd == -1. Seems like not much, but it is one more thing to remember.
  • SOCKET_TAKEOVER "just works" if fd == handle, otherwise we need some ugly-ass code like
// LibWebView/Process.cpp

#ifndef AK_OS_WINDOWS
    auto takeover_string = MUST(String::formatted("{}:{}", options.name, socket_fds[1]));
#else
    auto takeover_string = MUST(String::formatted("{}:{}", options.name, (intptr_t)Core::System::fd_to_handle(socket_fds[1])));
#endif

// LibCore/SystemServerTakeover.cpp

#ifndef AK_OS_WINDOWS
        s_overtaken_sockets.set(params[0].to_byte_string(), params[1].to_number<int>().value());
#else
        s_overtaken_sockets.set(params[0].to_byte_string(), handle_to_fd(params[1].to_number<intptr_t>().value(), System::SocketHandle));
#endif

The problem with CRT fds is that they are fake fds, they only work for some objects (files, folders) and certain circumstances (they cannot be inherited like handles).
So in the end they are too cumbersome to work with in our case.

So I decided to bite the bullet and consistently treat file descriptors as handles on Windows. Now, if you see an fd somewhere in the code outside of SystemWindows.cpp, it is always simply holds a handle on Windows.
Note: Windows handle is not an actual pointer, so it can safely be cast to int.

I was afraid that it will uglify SystemWindows.cpp, but this implementation turned out to be even shorter (by 40 lines). I am very pleased with the result.

There are only 3 places left in SystemWindows.cpp that use CRT fds: open, fstat and mmap. mmap requires fd, and with open/fstat I just wanted to use existing functions instead of implementing them manually using WIN32 APIs.
I use dup in these functions as advised here because there is no way of closing fd without closing underlying handle and handles that are associated with fds should not be closed with CloseHandle (link).
I renamed all fd arguments in SystemWindows.cpp to handle to distinguish them from CRT fds used in those 3 functions.

Also slightly improve Error::from_windows_error(int)
_O_OBTAIN_DIR flag makes _open use FILE_FLAG_BACKUP_SEMANTICS in
CreateFile call.

FILE_FLAG_BACKUP_SEMANTICS is required to open directory handles.

For ordinary files FILE_FLAG_BACKUP_SEMANTICS overrides file security
checks when the process has SE_BACKUP_NAME and SE_RESTORE_NAME
privileges, see https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea
Libraries/LibCore/SystemWindows.cpp Outdated Show resolved Hide resolved
Libraries/LibCore/SystemWindows.cpp Outdated Show resolved Hide resolved
@stasoid stasoid force-pushed the fd==handle branch 2 times, most recently from bf02e24 to 18dc2b0 Compare January 7, 2025 13:48
Also:
 * implement dup and is_socket
 * implement and call init_crt_and_wsa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants