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

Addition of core:os2/process api. #3310

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

flysand7
Copy link
Contributor

@flysand7 flysand7 commented Mar 22, 2024

This is a work-in-progress draft for the core:os2/process api. The goal is to provide an API to Odin for creating and running child processes and provide more things one might want from the API than libc provides.

Among some of the features planned:

  • Ability to run process given argv as an slice of strings, as opposed to a single shell command.
  • Ability to run processes via a command line (similarly to system() libc call)
  • Capturing std-streams (stdout, stderr, stdin) synchronously and asynchonously.
  • Waiting on the process to complete its work, or run asynchronously.
  • IPC and synchronization.
  • Asynchronous streams. To be able to check if a stream has data, wait on data, that is the functionality not available via synchronous streams.
  • Process debugging. The ability to start, step, inspect the state of a debugged process.

In the scope of this PR I will only implement the functionality for Windows and Linux, hopefully darwin won't be that different in terms of possible API interface.

@flysand7
Copy link
Contributor Author

flysand7 commented Mar 22, 2024

The get_stream procedure probably needs to be reworked. I realised that it's deadlock-prone if one doesn't check for availability of data on an input stream. If the child waits on input and we try to read (blocking) from an output stream, both process end up waiting for each other. And I reckon there won't be an API for waiting on streams for a while.

I'm considering python's subprocess.communicate API, since it provides a better API for performing a two-way communication with the child process.

@flysand7
Copy link
Contributor Author

There also needs to be a way to query file descriptors for process's current streams. The API will probably again end up like in Python. I should look at some other programming languages how they do it.

@flysand7
Copy link
Contributor Author

An ideal API for starting processes would be to just take argv array, since it allows to avoid the pain of quoting the command line arguments on per-platform basis. This is probably worse on windows, since the quoting behavior is a bit complicated and potentially non-trivial.

I'll leave the link here, since I'll probably need it later.

https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

@Lperlind
Copy link
Contributor

Also just for windows you'll probably want to use anonymous pipes which through the win32 api cannot be made to do overlapped IO. But you can totally just implement it yourself. A good reference is here: https://stackoverflow.com/questions/60645/overlapped-i-o-on-anonymous-pipe

@flysand7 flysand7 changed the title Addition of core:process api. Addition of core:os2/process api. Mar 23, 2024
@Yawning
Copy link
Contributor

Yawning commented Mar 24, 2024

On U*IX systems, please provide a method to allow the caller execute arbitrary code in the child between the fork and exec (mostly so I can call prctl a bunch in the child).

@flysand7 flysand7 mentioned this pull request Mar 25, 2024
@flysand7
Copy link
Contributor Author

On U*IX systems, please provide a method to allow the caller execute arbitrary code in the child between the fork and exec (mostly so I can call prctl a bunch in the child).

That's going to be a bit complicated to integrate into such an API that makes sense on multiple platforms. I'll see if I can make the unix version split into two functions to control the starting of the process.

@flysand7
Copy link
Contributor Author

Also can someone look at why core:crypto test is failing? I believe I haven't touched it

@Yawning
Copy link
Contributor

Yawning commented Mar 25, 2024

Also can someone look at why core:crypto test is failing? I believe I haven't touched it

Think this is an issue with the CI worker being flaky.

@Yawning
Copy link
Contributor

Yawning commented Mar 25, 2024

On U*IX systems, please provide a method to allow the caller execute arbitrary code in the child between the fork and exec (mostly so I can call prctl a bunch in the child).

That's going to be a bit complicated to integrate into such an API that makes sense on multiple platforms. I'll see if I can make the unix version split into two functions to control the starting of the process.

Add a suitably function pointer to Process_Desc that does nothing (or immediately returns an error if not nil, on process_open) on Windows/WASM? (Or hell, make Process_Desc platform specific).

My main use case is to be able to PR_SET_PDEATHSIG so that the OS handles auto-SIGTERMing the child when the parent dies. But having a boolean for that specific Linux-ism is even less ideal than "a function that gets called in the child-side after fork()", since that is potentially useful on every target that does fork + exec...

@laytan
Copy link
Sponsor Contributor

laytan commented Mar 25, 2024

Also can someone look at why core:crypto test is failing? I believe I haven't touched it

Think this is an issue with the CI worker being flaky.

It is an actual bug I am able to reproduce locally, not caused here though

@flysand7
Copy link
Contributor Author

My main use case is to be able to PR_SET_PDEATHSIG so that the OS handles auto-SIGTERMing the child when the parent dies. But having a boolean for that specific Linux-ism is even less ideal than "a function that gets called in the child-side after fork()", since that is potentially useful on every target that does fork + exec...

Having platform-specific extensions to Process_Desc struct doesn't seem that bad, I'll think about it. Callback function also sounds useful as well, in case there's more behavior to control. Before moving onto linux I'd want to make sure I understand how windows works, because it's a little bit more tricky API-wise, but API will probably change a lot in the following commits. And it'll keep changing once I start implementing the linux side

@flysand7
Copy link
Contributor Author

flysand7 commented Apr 16, 2024

Currently working slightly on the linux part of the API, and I think I've ran into some issues.

So in my current view of how the API should work like, you create the process by using process_open procedure, which will, under the hood call fork() and set up the child process (create the pipes, suspend etc). Then you call process_start() to resume the suspended process, which will call execve() starting the child process.

The issue revolves around suspending the process before running and returning the not found error, in case when the file to run wasn't found. Ideally this error should be returned by the process_open function, because that's where we establish the process to run. But we haven't ran execve yet, so we don't know whether to return this error yet.

I could do checks in process_open() to check whether the given file exists and has the right permissions, but that does introduce TOCTOU-style of issues, since the executable can be removed between the check and the time we run execve.

Which leads me to believe the issue here is the way I'm chosing to suspend the process. My idea was that upon creation child acquires a locking primitive or listens on a pipe until parent decides to wake it up, then it runs execve.

This isn't a repeatable way to do suspend/resume on a process, so maybe installing a signal handler that does the wait on parent is a better idea? Then I wonder whether it's possible to install a signal handler in the process_open procedure and send the sleep signal after execve in the child, but before any code in execve runs, in such a way that we can get execve error returned by the parent.

Of course another solution is to not provide "start suspended" API, if it cannot be implemented on linux in a sufficiently good way. Or introduce process_start as an alternative to process_resume which is only for the first time use.

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.

None yet

4 participants