Skip to content

Fix fork locking on win32 #1141

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

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Fix fork locking on win32 #1141

merged 2 commits into from
Apr 26, 2024

Conversation

G4Vi
Copy link
Collaborator

@G4Vi G4Vi commented Apr 11, 2024

Fixes #1138

  • __enable_threads / set __threaded in __proc_setup as threads are required for win32 subprocess management
  • move mmi/fds locking out of pthread_atfork.c into fork.c so it's done anytime __threaded is set instead of being dependent of pthreads
  • explicitly yoink _pthread_onfork_prepare, _pthread_onfork_parent, and _pthread_onfork_child in pthread_create.c so they are linked in in-case they are separated from _pthread_atfork

Big Thanks to @dfyz for help with locating the issue, testing, and devising a fix!

@G4Vi G4Vi marked this pull request as draft April 11, 2024 06:01
@G4Vi
Copy link
Collaborator Author

G4Vi commented Apr 11, 2024

Drafting this as it's incomplete, on win32 without pthreads it leaves the child unable to open files.

@G4Vi G4Vi marked this pull request as ready for review April 12, 2024 03:16
@G4Vi
Copy link
Collaborator Author

G4Vi commented Apr 12, 2024

Fixed the win32 non-pthread child not initializing the locks. Also decided to with __threaded always _pthread_lock/_pthread_unlock as they are also used by libc/calls/sig.c and libc/proc/execve-nt.greg.c regardless of pthreads being linked.

@mrdomino
Copy link
Collaborator

Why are threads required for win32 process management?

@G4Vi
Copy link
Collaborator Author

G4Vi commented Apr 25, 2024

I'm not really sure other than it's critical to the win32 fork implementation and is used to wait on child processes. __proc_lock calls __proc_setup which spawns a thread https://github.com/jart/cosmopolitan/blob/master/libc/proc/proc.c#L232-L236

Copy link
Collaborator

@mrdomino mrdomino left a comment

Choose a reason for hiding this comment

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

LGTM

@mrdomino
Copy link
Collaborator

The conflict came from clang-format line-breaking the short if statement; should be trivial to resolve.

G4Vi added 2 commits April 25, 2024 22:48
- __enable_threads / set __threaded in __proc_setup as threads are required for
  win32 subprocess management
- move mmi/fds locking out of pthread_atfork.c into fork.c so it's done anytime
  __threaded is set instead of being dependent of pthreads
- explicitly yoink _pthread_onfork_prepare, _pthread_onfork_parent, and
  _pthread_onfork_child in pthread_create.c so they are linked in in-case they
  are separated from _pthread_atfork

Big Thanks to @dfyz for help with locating the issue, testing, and devising a fix!
@G4Vi G4Vi force-pushed the fix-win32-fork-locking branch from 0b236a9 to 4b39d15 Compare April 26, 2024 02:48
@G4Vi G4Vi requested a review from mrdomino April 26, 2024 02:48
@G4Vi G4Vi merged commit 69db501 into jart:master Apr 26, 2024
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.

[windows] fork() ReadFile_SIZE_CHECK() failed with win32 code 0
2 participants