This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9058

chg: fallback to original hg if stdio fds are missing
ClosedPublic

Authored by quark on Sep 18 2020, 8:07 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG5eee6f4f3d0d: chg: fallback to original hg if stdio fds are missing
Summary

If stdio fds are missing (ex. fd 0 is not present), chg might open
fds that take the numbers 0, and attachio would send the wrong fds
to the client, which might cause unwanted behaviors. Avoid that by
detecting the missing fds and falling back to the original hg.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Sep 18 2020, 8:07 PM
indygreg accepted this revision.Sep 22 2020, 10:10 PM
indygreg added a subscriber: indygreg.

This looks like a legit bug fix. And when I see patches like this, I wonder what circumstances led to its discovery :p

This revision is now accepted and ready to land.Sep 22 2020, 10:10 PM
quark added a commit: rHG5eee6f4f3d0d: chg: fallback to original hg if stdio fds are missing.Sep 22 2020, 10:23 PM
Closed by commit rHG5eee6f4f3d0d: chg: fallback to original hg if stdio fds are missing (authored by quark). · Explain Why
This revision was automatically updated to reflect the committed changes.
quark added a comment.EditedSep 27 2020, 9:10 PM
In D9058#135940, @indygreg wrote:

This looks like a legit bug fix. And when I see patches like this, I wonder what circumstances led to its discovery :p

Context here. There was a similar issue in watchman. It seems this can be an easy "mistake" using the posix_spawn API.

Revision Contents
Changeset List

PathPackages
Mcontrib/chg/chg.c (18 lines)
Mtests/test-chg.t (8 lines)

Diff 22762

contrib/chg/chg.c

} else { } else {
abortmsg("unknown instruction: %s", *pinst); abortmsg("unknown instruction: %s", *pinst);
} }
} }
return needreconnect; return needreconnect;
} }
/* /*
* Test whether the command is unsupported or not. This is not designed to * Test whether the command and the environment is unsupported or not.
* cover all cases. But it's fast, does not depend on the server. *
* If any of the stdio file descriptors are not present (rare, but some tools
* might spawn new processes without stdio instead of redirecting them to the
* null device), then mark it as not supported because attachio won't work
* correctly.
*
* The command list is not designed to cover all cases. But it's fast, and does
* not depend on the server.
*/ */
static int isunsupported(int argc, const char *argv[]) static int isunsupported(int argc, const char *argv[])
{ {
enum { SERVE = 1, enum { SERVE = 1,
DAEMON = 2, DAEMON = 2,
SERVEDAEMON = SERVE | DAEMON, SERVEDAEMON = SERVE | DAEMON,
}; };
unsigned int state = 0; unsigned int state = 0;
int i; int i;
/* use fcntl to test missing stdio fds */
if (fcntl(STDIN_FILENO, F_GETFD) == -1 ||
fcntl(STDOUT_FILENO, F_GETFD) == -1 ||
fcntl(STDERR_FILENO, F_GETFD) == -1) {
debugmsg("stdio fds are missing");
return 1;
}
for (i = 0; i < argc; ++i) { for (i = 0; i < argc; ++i) {
if (strcmp(argv[i], "--") == 0) if (strcmp(argv[i], "--") == 0)
break; break;
/* /*
* there can be false positives but no false negative * there can be false positives but no false negative
* we cannot assume `serve` will always be first argument * we cannot assume `serve` will always be first argument
* because global options can be passed before the command name * because global options can be passed before the command name
*/ */

tests/test-chg.t

$ chg bulkwrite --pager=on --color no --config ui.formatted=True $ chg bulkwrite --pager=on --color no --config ui.formatted=True
paged! 'going to write massive data\n' paged! 'going to write massive data\n'
killed! (?) killed! (?)
[255] [255]
$ cd .. $ cd ..
missing stdio
-------------
$ CHGDEBUG=1 chg version -q 0<&-
chg: debug: * stdio fds are missing (glob)
chg: debug: * execute original hg (glob)
Mercurial Distributed SCM * (glob)
server lifecycle server lifecycle
---------------- ----------------
chg server should be restarted on code change, and old server will shut down chg server should be restarted on code change, and old server will shut down
automatically. In this test, we use the following time parameters: automatically. In this test, we use the following time parameters:
- "sleep 1" to make mtime different - "sleep 1" to make mtime different
- "sleep 2" to notice mtime change (polling interval is 1 sec) - "sleep 2" to notice mtime change (polling interval is 1 sec)