Shore Up File System Interactions to Avoid Unexpected Outcomes

[rmartin16]

Changes

• Uses of subprocess with user-input derived filepaths to executables now catch OSError in case those executables have unepxected aspects that causes subprocess to raise variations of OSError.
• Checks for filesystem existence that also assumed it was either a file or a directory now explicitly check for that characteristic.
• As a specific example, if the filepath in JAVA_HOME is not exlusively made up of directories, then subprocess will raise NotADirectoryError. For instance, if JAVA_HOME points to a binary itself such as /usr/lib/jvm/bin/java, then the check for javac, i.e. subprocess.check_output(['/usr/lib/jvm/bin/java/bin/javac', '-version']) raises NotADirectoryError since java is not a directory.

I was not previously aware of this behavior from subprocess. It seems to stem from the fact that the state returned from forking the child is an OSError number 0x14.....instead of OSError number 0x2 for FileNotFoundError, for instance.

This is simple to replicate, though:

>>> import subprocess
>>> subprocess.check_output(['/bin/sh/asdf'])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/russell/.pyenv/versions/3.10.9-debug/lib/python3.10/subprocess.py", line 421, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/home/russell/.pyenv/versions/3.10.9-debug/lib/python3.10/subprocess.py", line 503, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/home/russell/.pyenv/versions/3.10.9-debug/lib/python3.10/subprocess.py", line 971, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/home/russell/.pyenv/versions/3.10.9-debug/lib/python3.10/subprocess.py", line 1847, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
NotADirectoryError: [Errno 20] Not a directory: '/bin/sh/asdf'

TODO:

• Identify other uses of subprocess with potentially problematic exe filepaths.
  • The most vulnerable are those filepaths composed from user/external input.

Reference:

• Beewrre Tutorial Step 5: can't find folder beeware#198

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

Thanks for getting ahead of this one. Based on the original report, I figured the fix would be something like this.

[rmartin16]

Another approach here is to catch OSError generally....and even directly within integrations.subprocess.Subprocess.

If subprocess raises an OSError exception or one of its subclasses (since it does that through OSError.__new__() magic), something particularly bad happened. OTOH, we've definitely decided in some cases that it is better to spill the exception in to the console so it's hopefully clearer to the user something really bad did happen.

[rmartin16]

There are already several places we're checking for FileNotFoundError....so, the middle ground is probably updating those to catch OSError more broadly instead so that any filesystem interaction errors are managed.

Basically, if we're trying to interact with a file or directory and it fails with an exception based on OSError, I believe the situation is the same; we need to let the user know this location is inaccessible (for whatever reason) and abort the entire command (or move on in some cases).

Additionally, we should probably ensure this policy applies to any interaction with "non-well-known" filesystem structures...or at least anywhere a user could reasonably have made their own changes. So, once a well-known tool is validated (like Xcode, java, android sdk, etc), we shouldn't need to worry about this....but trying to run open("/random/user/file") or subprocess.run("/user/specified/exe"), this exception management should probably be in place.

[freakboy3742]

Not much to fault here - this is a pretty comprehensive audit.

I'm sure we could write a bunch more tests to individually confirm that "file is directory" type failures do indeed fail; and there's probably some really subtle masking condition where a weird permission error will be surfaced as a "does not exist". However, it's enough of an edge case that I'm not too concerned - if we ever get a user report of something like that happening, we can deal with it then.