Details

Reviewers

Maniphest Tasks

T10853: Make `setCWD('/does/not/exist')` safer on ExecFuture

Commits

rPHU0679917944e1: Refine checks against "cwd" before "proc_open()"

Summary

See PHI1094. Previously, see D15785. Fixes T10853. Upstream: https://bugs.php.net/bug.php?id=77656.

PHP allows you to proc_open() into any CWD, including one that does not exist, is not readable, and/or is not executable.

We attempt to catch this, but don't do enough checks, and allow you to attempt execution into a directory which exists but which you can't read or execute.

Add more checks to prevent this. Also, try to make assertExists() more clever about cases where a path really doesn't exist versus cases where we aren't allowed to tell if a path exists or not because of a permissions issue.

(It's technically legal to chdir() into a directory you only have +x on, although this is probably ill-advised.)

Test Plan

Used this script with various arguments:

<?php

require_once '/Users/epriestley/dev/phabricator/scripts/init/init-script.php';

$result = id(new ExecFuture('pwd && ls -alh'))
  ->setCWD($argv[1])
  ->resolvex();

var_dump($result);

In all cases, the directory name describes the permissions. subdir/ exists and is a directory we have "+r" and "+x" on.

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable
[2019-02-22 15:34:14] EXCEPTION: (Exception) Preparing to run a command in directory "/Users/epriestley/scratch/not-readable", but that directory is not readable (the current process does not have "+r" permission). at [<phutil>/src/future/exec/PhutilExecutableFuture.php:156]

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable/subdir
array(2) {
  [0]=>
  string(163) "/Users/epriestley/scratch/not-readable/subdir
total 0
drwxr-xr-x  2 epriestley  staff    64B Feb 22 15:33 .
drwxr-x--x  3 root        wheel    96B Feb 22 15:33 ..
"
  [1]=>
  string(0) ""
}

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-executable
[2019-02-22 15:34:24] EXCEPTION: (Exception) Preparing to run a command in directory "/Users/epriestley/scratch/not-executable", but that directory is not executable (the current process does not have "+x" permission). at [<phutil>/src/future/exec/PhutilExecutableFuture.php:165]

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-executable/subdir
[2019-02-22 15:34:26] EXCEPTION: (PhutilProxyException) Unable to run a command in directory "/Users/epriestley/scratch/not-executable/subdir". {>} (FilesystemException) Filesystem path "/Users/epriestley/scratch/not-executable/subdir" can not be accessed because a parent directory ("/Users/epriestley/scratch/not-executable") is not executable (the current process does not have "+x" permission). at [<phutil>/src/filesystem/Filesystem.php:1142]

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable-or-executable/
[2019-02-22 15:34:32] EXCEPTION: (Exception) Preparing to run a command in directory "/Users/epriestley/scratch/not-readable-or-executable/", but that directory is not readable (the current process does not have "+r" permission). at [<phutil>/src/future/exec/PhutilExecutableFuture.php:156]

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable-or-executable/subdir
[2019-02-22 15:34:33] EXCEPTION: (PhutilProxyException) Unable to run a command in directory "/Users/epriestley/scratch/not-readable-or-executable/subdir". {>} (FilesystemException) Filesystem path "/Users/epriestley/scratch/not-readable-or-executable/subdir" can not be accessed because a parent directory ("/Users/epriestley/scratch/not-readable-or-executable") is not executable (the current process does not have "+x" permission). at [<phutil>/src/filesystem/Filesystem.php:1142]

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/does-not-exist
[2019-02-22 15:34:37] EXCEPTION: (PhutilProxyException) Unable to run a command in directory "/Users/epriestley/scratch/does-not-exist". {>} (FilesystemException) Filesystem path "/Users/epriestley/scratch/does-not-exist" does not exist. at [<phutil>/src/filesystem/Filesystem.php:1153]

Diff Detail

Repository

rPHU libphutil

Branch

subexec1

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/filesystem/Filesystem.php:1145	XHP62	Class Name Literal
Advice	src/filesystem/Filesystem.php:1156	XHP62	Class Name Literal

Unit

Tests Passed

Build Status

Buildable 22098
Build 30188: Run Core Tests
Build 30187: arc lint + arc unit

Event Timeline

epriestley created this revision.Feb 22 2019, 11:40 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 22 2019, 11:40 PM

Harbormaster failed remote builds in B22097: Diff 48240!

Builds are failing in a curious way so this probably breaks something, let me see if I can figure out what..

src/filesystem/Filesystem.php
1145	Actual example of ">>" working properly, above. ^^^

Set my "origin" back to the right value after testing git remote set-url commands with random values.

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 22 2019, 11:50 PM

Harbormaster failed remote builds in B22098: Diff 48241!

Remove local code that made "arc diff" change the working copy's origin URL.

Harbormaster completed remote builds in B22099: Diff 48242.Feb 22 2019, 11:52 PM

epriestley requested review of this revision.Feb 22 2019, 11:52 PM

o_o

Also this:

Screen Shot 2019-02-22 at 7.38.34 PM.png (530×982 px, 41 KB)

This revision is now accepted and ready to land.Feb 23 2019, 2:39 AM

epriestley edited the summary of this revision. (Show Details)Feb 23 2019, 2:43 AM

I'm going to cut the release before trying to unleash this, and I also want to make sure is_executable($dir) works in a similar way on Windows.

It's possible the behavior is, say, "always return false", in which case I'll add if (windows) { do nothing } else { check execute } sorts of guards.

On Windows, I believe the call chain is is_executable(...) → VCWD_ACCESS(..., X_OK) → tsrm_win32_access(..., X_OK) → php_win32_ioutil_access_w(..., X_OK) → GetBinaryTypeW(...).

This method tests if a path is a .exe: https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getbinarytypew

This suggests is_executable(...) will always return false for directories on Windows. As far as I can tell, this is also the actual behavior.

I'm going to skip the is_executable(..) checks under Windows for now since they definitely don't work as written (as far as I can tell, they always fail). If we discover there's some secret alternative magic in the future, we can specialize things further.

On Windows, don't do "+x" checks against directories because these checks seem to always fail and not be meaningful anyway.

Harbormaster completed remote builds in B22120: Diff 48258.Feb 25 2019, 8:04 PM

Closed by commit rPHU0679917944e1: Refine checks against "cwd" before "proc_open()" (authored by epriestley). · Explain WhyFeb 25 2019, 8:14 PM

This revision was automatically updated to reflect the committed changes.

epriestley mentioned this in T13565: Filesystem write failure error messages lack tailoring, notably in DeferredLog.Aug 11 2020, 6:04 PM

Refine checks against "cwd" before "proc_open()"
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 48241

src/filesystem/Filesystem.php

src/future/exec/PhutilExecutableFuture.php

Refine checks against "cwd" before "proc_open()"ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 48241

src/filesystem/Filesystem.php

src/future/exec/PhutilExecutableFuture.php

Refine checks against "cwd" before "proc_open()"
ClosedPublic
Actions

Revision Contents
Changeset List