Page MenuHomePhabricator
Files F14109661

D20616.diff
No OneTemporary
Actions

D20616.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -365,6 +365,7 @@
'PhutilProcessGroupDaemon' => 'daemon/torture/PhutilProcessGroupDaemon.php',
'PhutilProcessQuery' => 'filesystem/PhutilProcessQuery.php',
'PhutilProcessRef' => 'filesystem/PhutilProcessRef.php',
+ 'PhutilProcessRefTestCase' => 'filesystem/__tests__/PhutilProcessRefTestCase.php',
'PhutilProseDiff' => 'utils/PhutilProseDiff.php',
'PhutilProseDiffTestCase' => 'utils/__tests__/PhutilProseDiffTestCase.php',
'PhutilProseDifferenceEngine' => 'utils/PhutilProseDifferenceEngine.php',
@@ -1031,6 +1032,7 @@
'PhutilProcessGroupDaemon' => 'PhutilTortureTestDaemon',
'PhutilProcessQuery' => 'Phobject',
'PhutilProcessRef' => 'Phobject',
+ 'PhutilProcessRefTestCase' => 'PhutilTestCase',
'PhutilProseDiff' => 'Phobject',
'PhutilProseDiffTestCase' => 'PhutilTestCase',
'PhutilProseDifferenceEngine' => 'Phobject',
diff --git a/src/filesystem/PhutilProcessQuery.php b/src/filesystem/PhutilProcessQuery.php
--- a/src/filesystem/PhutilProcessQuery.php
+++ b/src/filesystem/PhutilProcessQuery.php
@@ -36,13 +36,10 @@
list($pid, $command) = $parts;
$ref = id(new PhutilProcessRef())
- ->setPID((int)$pid)
- ->setCommand($command);
+ ->setPID((int)$pid);
- // If this process is a "phd-daemon" process, mark it as a daemon
- // overseer.
- $is_overseer = (bool)preg_match('/\bphd-daemon\b/', $command);
- $ref->setIsOverseer($is_overseer);
+ $argv = $this->getArgv($pid, $command);
+ $ref->setArgv($argv);
// If this is an overseer and the command has a "-l" ("Label") argument,
// the argument contains the "PHABRICATOR_INSTANCE" value for the daemon.
@@ -80,4 +77,49 @@
return array_values($refs);
}
+ private function getArgv($pid, $command) {
+
+ // In the output of "ps", arguments in process titles are not escaped, so
+ // we can not distinguish between the processes created by running these
+ // commands by looking only at the output of "ps":
+ //
+ // echo 'a b'
+ // echo a b
+ //
+ // Both commands will have the same process title in the output of "ps".
+
+ // This means we may split the command incorrectly in the general case,
+ // and this misparsing may be important if the process binary resides in
+ // a directory with spaces in its path and we're trying to identify which
+ // binary a process is running.
+
+ // On Ubuntu, and likely most other Linux systems, we can get a raw
+ // command line from "/proc" with arguments delimited by "\0".
+
+ // On macOS, there's no "/proc" and we don't currently have a robust way
+ // to split the process command in a way that parses spaces properly, so
+ // fall back to a best effort based on the output of "ps". This is almost
+ // always correct, since it is uncommon to put binaries under paths with
+ // spaces in them.
+
+ $proc_cmdline = sprintf('/proc/%d/cmdline', $pid);
+ try {
+ $argv = Filesystem::readFile($proc_cmdline);
+ $argv = explode("\0", $argv);
+
+ // The output itself is terminated with "\0", so remove the final empty
+ // argument.
+ if (last($argv) === '') {
+ array_pop($argv);
+ }
+
+ return $argv;
+ } catch (Exception $ex) {
+ // If we fail to read "/proc", fall through to less reliable methods.
+ }
+
+ // If we haven't found a better source, just split the "ps" output on
+ // spaces.
+ return preg_split('/\s+/', $command);
+ }
}
diff --git a/src/filesystem/PhutilProcessRef.php b/src/filesystem/PhutilProcessRef.php
--- a/src/filesystem/PhutilProcessRef.php
+++ b/src/filesystem/PhutilProcessRef.php
@@ -7,6 +7,7 @@
private $command;
private $isOverseer;
private $instance;
+ private $argv;
public function setPID($pid) {
$this->pid = $pid;
@@ -17,21 +18,23 @@
return $this->pid;
}
- public function setCommand($command) {
- $this->command = $command;
- return $this;
- }
-
public function getCommand() {
- return $this->command;
- }
+ if (!$this->command) {
+ $this->command = phutil_string_cast(csprintf('%LR', $this->argv));
+ }
- public function setIsOverseer($is_overseer) {
- $this->isOverseer = $is_overseer;
- return $this;
+ return $this->command;
}
public function getIsOverseer() {
+ if ($this->isOverseer === null) {
+ $this->isOverseer = $this->getCommandMatch(
+ array(
+ array('phd-daemon'),
+ array('php', 'phd-daemon'),
+ ));
+ }
+
return $this->isOverseer;
}
@@ -44,4 +47,34 @@
return $this->instance;
}
+ private function getCommandMatch(array $patterns) {
+ $argv = $this->getArgv();
+
+ foreach ($patterns as $pattern) {
+ $pattern = array_values($pattern);
+ $is_match = true;
+ for ($ii = 0; $ii < count($pattern); $ii++) {
+ if (basename($argv[$ii]) !== $pattern[$ii]) {
+ $is_match = false;
+ break;
+ }
+ }
+
+ if ($is_match) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ public function setArgv(array $argv) {
+ $this->argv = $argv;
+ return $this;
+ }
+
+ public function getArgv() {
+ return $this->argv;
+ }
+
}
diff --git a/src/filesystem/__tests__/PhutilProcessRefTestCase.php b/src/filesystem/__tests__/PhutilProcessRefTestCase.php
new file mode 100644
--- /dev/null
+++ b/src/filesystem/__tests__/PhutilProcessRefTestCase.php
@@ -0,0 +1,59 @@
+<?php
+
+final class PhutilProcessRefTestCase
+ extends PhutilTestCase {
+
+ public function testIdentifyOverseerProcess() {
+
+ // Test if various process argument vectors are correctly identified as
+ // daemon overseer processes or not. We're hoping to identify legitimate
+ // daemons and ignore false positives for processes with titles that look
+ // similar but are not really daemons, like "grep phd-daemon".
+
+ $tests = array(
+ array(
+ array('php', 'phd-daemon'),
+ true,
+ ),
+ array(
+ array('/path/to/php', '/path/to/phd-daemon'),
+ true,
+ ),
+ array(
+ array('/path/to/phd-daemon'),
+ true,
+ ),
+ array(
+ array('phd-daemon'),
+ true,
+ ),
+ array(
+ array('php', 'phd-daemon', '-l', 'instance-label'),
+ true,
+ ),
+ array(
+ array('grep phd-daemon'),
+ false,
+ ),
+ array(
+ array('this-is-a-phd-daemon'),
+ false,
+ ),
+ );
+
+ foreach ($tests as $case) {
+ list($argv, $expect) = $case;
+
+ $ref = id(new PhutilProcessRef())
+ ->setArgv($argv);
+
+ $actual = $ref->getIsOverseer();
+
+ $this->assertEqual(
+ $expect,
+ $actual,
+ pht('argv: %s', implode(' ', $argv)));
+ }
+ }
+
+}

File Metadata

Mime Type
text/plain
Expires
Thu, Nov 28, 3:00 PM (21 h, 3 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6801005
Default Alt Text
D20616.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility