Patchwork [1,of,2] pager: avoid shell=True on subprocess.Popen for better errors (issue5491)

login
register
mail settings
Submitter Augie Fackler
Date March 16, 2017, 1:44 a.m.
Message ID <07d488f16da6e12b225d.1489628665@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/19372/
State Accepted
Headers show

Comments

Augie Fackler - March 16, 2017, 1:44 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1489624427 14400
#      Wed Mar 15 20:33:47 2017 -0400
# Node ID 07d488f16da6e12b225d2827f1020f32c8050a7a
# Parent  1c48a8278b2f015fca607dfc652823560a5ac580
pager: avoid shell=True on subprocess.Popen for better errors (issue5491)

man(1) behaves as poorly as Mercurial without this change. This cribs
from git's run-command[0], which has a list of characters that imply a
string that needs to be run using 'sh -c'. If none of those characters
are present in the command string, we can use shell=False mode on
subprocess and get significantly better error messages (see the test)
when the pager process is invalid. With a complicated pager command
(that contains one of the unsafe characters), we behave as we do today
(which is no worse than git manages.)

I briefly tried tapdancing in a thread to catch early pager exits, but
it's just too perilous: you get races between fd duping operations and
a bad pager exiting, and it's too hard to differentiate between a
slow-bad-pager result and a fast-human-quit-pager-early result.

I've observed some weird variation in exit code handling in the "bad
experience" case in test-pager.t: on my Mac hg predictably exits
nonzero, but on Linux hg always exits zero in that case. For now,
we'll work around it with || true. :(

0: https://github.com/git/git/blob/cddbda4bc87b9d2c985b6749b1cf026b15e2d3e7/run-command.c#L201

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -935,9 +935,22 @@  class ui(object):
         This is separate in part so that extensions (like chg) can
         override how a pager is invoked.
         """
-        pager = subprocess.Popen(command, shell=True, bufsize=-1,
-                                 close_fds=util.closefds, stdin=subprocess.PIPE,
-                                 stdout=util.stdout, stderr=util.stderr)
+        # If the command doesn't contain any of these characters, we
+        # assume it's a binary and exec it directly. This means for
+        # simple pager command configurations, we can degrade
+        # gracefully and tell the user about their broken pager.
+        shell = any(c in command for c in "|&;<>()$`\\\"' \t\n*?[#~=%")
+        try:
+            pager = subprocess.Popen(
+                command, shell=shell, bufsize=-1,
+                close_fds=util.closefds, stdin=subprocess.PIPE,
+                stdout=util.stdout, stderr=util.stderr)
+        except OSError as e:
+            if e.errno == errno.ENOENT and not shell:
+                self.warn(_("missing pager command '%s', skipping pager\n")
+                          % command)
+                return
+            raise
 
         # back up original file descriptors
         stdoutfd = os.dup(util.stdout.fileno())
diff --git a/tests/test-pager.t b/tests/test-pager.t
--- a/tests/test-pager.t
+++ b/tests/test-pager.t
@@ -121,6 +121,33 @@  even though stdout is no longer a tty.
   paged! 'summary:     modify a 8\n'
   paged! '\n'
 
+An invalid pager command name is reported sensibly if we don't have to
+use shell=True in the subprocess call:
+  $ hg log --limit 3 --config pager.pager=this-command-better-never-exist
+  missing pager command 'this-command-better-never-exist', skipping pager
+  \x1b[0;33mchangeset:   10:46106edeeb38\x1b[0m (esc)
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     modify a 10
+  
+  \x1b[0;33mchangeset:   9:6dd8ea7dd621\x1b[0m (esc)
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     modify a 9
+  
+  \x1b[0;33mchangeset:   8:cff05a6312fe\x1b[0m (esc)
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     modify a 8
+  
+
+A complicated pager command gets worse behavior. Bonus points if you can
+improve this.
+  $ hg log --limit 3 \
+  >   --config pager.pager='this-command-better-never-exist --seriously' \
+  >  2>/dev/null || true
+
 Pager works with shell aliases.
 
   $ cat >> $HGRCPATH <<EOF