Page MenuHomePhabricator

Make `setCWD('/does/not/exist')` safer on ExecFuture
Closed, ResolvedPublic

Description

When you run a command with setCWD(...) (which ultimately resolves to a cwd parameter to proc_open()) and the process is unable to switch to the specified cwd, the command runs in the current CWD without raising any errors.

This script lists the current directory:

epriestley@orbital ~ $ cat test.php 
<?php

$spec = array();
$pipes = array();

$result = proc_open(
  'ls',
  $spec,
  $pipes,
  '/does/not/exist');

var_dump($result);
epriestley@orbital ~ $ php -f test.php 
resource(4) of type (process)
ABBYY			Desktop			Dropbox			Music			dbdump.db		out.2			test.php
Applications		Documents		Library			Pictures		dev			scans
Creative Cloud Files	Downloads		Movies			Public			out.1			src

Normally this is okay-ish because most commands just fail when run in the wrong directory, but some commands (like git fetch +refs/*:refs/* or rm ...) may be very dangerous.

There doesn't seem to be any way to detect or prevent this that I can see. Ideally, proc_open would fail abruptly if changing the CWD failed. Two things we could do:

  • Test if the CWD exists before running the command. It's still possible that the directory is removed between our check and the proc_open().
  • Explicitly chdir() somewhere "safe" before running any CWD commands, so if they mess up they're less likely to destroy anything? This gets complicated because it's a side effect that the caller may not really expect or anticipate. We (probably?) don't rely on on CWD anywhere in web Phabricator, but the various CLI scripts do.