ProcessMonitor is too restrictive with execute bit check

[cararemixed]

Currently we have ProcessMonitor checking that execute is set for all users (not just the user or group):

https://github.com/ponylang/ponyc/blob/0.22.1/packages/process/process_monitor.pony#L245

As pointed out in IRC, this could cause problems on systems with more advanced ACLs and also cause unneeded restriction when a user has proper ownership for execution via the other two execute bits (commonly the only ones set via the default session umask).

The original change for this code was there to require authority in order to execute something. This seems to work fine but I'm not sure why we also check these bits since the host system will be doing these checks as well (and more accurately).

I'd suggest we either remove the check or otherwise return a more specific error type since it's not the execve call itself that is failing and add documentation for this specific restriction.

[jemc]

If we remove the check, and let the OS check the executable bit, what kind of error will we see in case of those OS checks failing?

[cararemixed]

Unfortunately we get no error. Right now the parent process does not call anything upon the child's exit, so we don't get the -1 exit code that the code currently returns from the child after forking but failing to exec. It seems like the ProcessNotifier interface is not working as intended.

It might be worth checking that the file exists but I'm not sure if there is an exotic case where we can't stat a file but can exec it.

I'm looking at what other runtimes do here and it doesn't always seem to be consistent. At the very least, our notifier should have a method of detecting that the child process has exited and what the code is.

[mfelsche]

python for example uses a pipe to signal an error in the child process: https://github.com/python/cpython/blob/e756f66c83786ee82f5f7d45931ae50a6931dd7f/Lib/subprocess.py#L1389

The model here could be to write the return code of execve to the pipe, which only happens on error. On success there is nothing to read.
I think it is a good idea to extend ProcessNotify to be notified here of an execve failure that is distinct of the return code of the process that is intended to be run.

The current check is incomplete and, looking at the manpage of execve, there are a lot more error conditions even with the executable bit for the current user being set correctly. e.g. the file being on a file system mounted with the noexec option. Thus the only interesting check we can and should do is for existence of the file. All other error conditions should be catched using a "error-pipe" to the child process and sent to the ProcessNotify.

[cararemixed]

@mfelsche I agree here. It seems like this is a good path forward.

To break this up into a couple steps, I think changing the local code to check that the file exists is a good sanity check for now. In order to keep compatibility, I can leave the current error code in place since it's unused otherwise.

Moving forward from there, we can use the additional pipe. I can look a bit closer at what we already have but it shouldn't be hard to add the pipe. The other trick will be to fix some of the otherwise impossible notifications. I'd really like to get an exit notification with a code working properly. This might be a bigger and more complex patch so I'm in favor of doing it separately.

[SeanTAllen]

@strmpnk does #2717 close this?

[jemc]

I feel like the PR does close this issue, but we should open another ticket for the piped exit code improvement. Do you agree, @strmpnk?

[cararemixed]

I was going to keep this issue for addressing the follow up but I'm happy to close this and create an follow up issue which references this.