Page MenuHomePhabricator

Setting "phd.trace" may cause TTY testing with "posix_isatty()" against closed pipes
Open, WishlistPublic

Description

See https://discourse.phabricator-community.org/t/cant-run-daemons-with-phd-trace/4111/.

If you set phd.trace, we may eventually call posix_isatty(STDOUT) from a context where STDOUT exists but has been closed.

Although the posix_isatty() tests could likely be refined, phd.trace also seems like it isn't very useful in modern Phabricator (I haven't used it diagnostically in many years) and it can probably just be removed.

Event Timeline

epriestley triaged this task as Wishlist priority.
epriestley created this task.

In D21426, I removed phd.trace and phd.verbose.

We may still reach calls to posix_isatty(STDOUT) and possibly posix_isatty(STDERR) after those pipes have closed. It's possible we can also reach posix_isatty(STDIN) with a closed stdin.

See also phutil_is_noninteractive() and phutil_is_interactive(), but the various posix_isatty() tests are really testing different things:

  • Can we prompt the user interactively?
  • Should we pipe output to a pager?
  • Should we use ANSI console codes?
  • Can/should we draw fancy high-I/O graphics (e.g. progress bars)?
  • When daemonizing, should we close STDOUT/STDERR? (This is specific to HgProxyServer and attempting to test for "have they been redirected".)

All this stuff should probably (?) move to some ConsoleCapabilities class alongside phutil_console_get_terminal_width() and the broader ANSI support tests. A trick is that many of these tests have a common "we can't tell" state, and callers often want different default behaviors.

Another open question is: before we call posix_isatty(STDOUT), how do we test whether STDOUT is a valid handle?