monte-language / typhon

A virtual machine for Monte.
Other
67 stars 10 forks source link

makeProcess should not auto-search PATH #155

Closed justinnoah closed 4 years ago

justinnoah commented 7 years ago
MostAwesomeDude commented 7 years ago

I completely agree that we should fix any buggy makeProcess behavior. Without testcases, I'm going to just push forward to talk about handling PATH sanely.

My instinct here is to petname binaries via a configurable mapping that the Typhon binary automatically generates by browsing PATH. That would be a pretty cushy layer of foam padding that nonetheless allows for some good behaviors.

To use it, there would be several options. Suppose "/usr/bin/yes" exists and is on our PATH. First, a direct invocation on the primitive makeProcess:

def proc := makeProcess("yes", [], [].asMap())

This screams for attenuation:

def makeProc := makeProcess.withExecutable("yes")
def proc := makeProc([], [].asMap())

At this point, we wonder whether the argument and environment variables should really be required on every invocation. I suggest that they should stay, since they directly correspond to control over the entrypoint of the executable, and in practical situations, they won't be empty.

I have put strings here instead of bytes for the binary names. This is in keeping with our goal to talk about paths as bytes only when absolutely necessary, and frankly I'm not sure I want to encourage folks to run executables with strange difficult-to-handle names.

washort commented 7 years ago

Strongly in favor of separating access to PATH from access to process invocation. Shouldn't be too hard to provide a which function that searches PATH for you.

MostAwesomeDude commented 6 years ago

As you can tell, makeProcess likes to cheat already:

▲> def p := makeProcess(b`bash`, [], [].asMap(), "stderr" => true, "stdout" => true)
[pid 10715] clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f55bd84a350) =4
strace: Process 10724 attached
[pid 10724] execve("/run/current-system/sw/bin/bash", [], 0x2834330 /* 0 vars */) = 0

Good news: The original desired behavior is present! Bad news: We don't want that. So we should do something about it.

MostAwesomeDude commented 6 years ago

From the man page:

The file is sought in the colon-separated list of directory pathnames specified in the PATH environment variable. If this variable isn't defined, the path list defaults to the current directory followed by the list of directories returned by confstr(_CS_PATH).

Python:

>>> os.confstr("CS_PATH")
'/run/current-system/sw/bin:/bin:/usr/bin'

This is a pretty good default! But unfortunately it means that, even if we blank PATH, we'll still have it returned to us.