| Submitter | Yuya Nishihara |
|---|---|
| Date | Nov. 15, 2014, 10:34 a.m. |
| Message ID | <15b63d55d18198a1e262.1416047667@mimosa> |
| Download | mbox | patch |
| Permalink | /patch/6744/ |
| State | Accepted |
| Commit | 69f86b9370350a939ed415f49e2ff110085d447d |
| Headers | show |
Comments
On 11/15/14 2:34 AM, Yuya Nishihara wrote: > # HG changeset patch > # User Yuya Nishihara <yuya@tcha.org> > # Date 1416027043 -32400 > # Sat Nov 15 13:50:43 2014 +0900 > # Node ID 15b63d55d18198a1e2621c79419b7b1f66a8875e > # Parent d6e1a0cd0da71ebd777d2f35a52b6390ca3915e8 > cmdserver: protect pipe server streams against corruption caused by direct io > > Because pipe-mode server uses stdio as IPC channel, other modules should not > touch stdio directly and use ui instead. However, this strategy is brittle > because several Python functions read and write stdio implicitly. > > print 'hello' # should use ui.write() > # => ch = 'h', size = 1701604463 'ello', data = '\n' > > This patch adds protection for such mistakes. Both stdio files and low-level > file descriptors are redirected to /dev/null while command server uses them. Did you consider installing a wrapper object into sys.std* that converts raw I/O into the proper format and having the pipe server and ui write into the "real" fds? That would seem like a safer and more robust approach, especially for people with extensions using 3rd party libraries that may not send all stdio through Mercurial APIs.
On Sat, 15 Nov 2014 08:40:15 -0800, Gregory Szorc wrote: > On 11/15/14 2:34 AM, Yuya Nishihara wrote: > > # HG changeset patch > > # User Yuya Nishihara <yuya@tcha.org> > > # Date 1416027043 -32400 > > # Sat Nov 15 13:50:43 2014 +0900 > > # Node ID 15b63d55d18198a1e2621c79419b7b1f66a8875e > > # Parent d6e1a0cd0da71ebd777d2f35a52b6390ca3915e8 > > cmdserver: protect pipe server streams against corruption caused by direct io > > > > Because pipe-mode server uses stdio as IPC channel, other modules should not > > touch stdio directly and use ui instead. However, this strategy is brittle > > because several Python functions read and write stdio implicitly. > > > > print 'hello' # should use ui.write() > > # => ch = 'h', size = 1701604463 'ello', data = '\n' > > > > This patch adds protection for such mistakes. Both stdio files and low-level > > file descriptors are redirected to /dev/null while command server uses them. > > Did you consider installing a wrapper object into sys.std* that converts > raw I/O into the proper format and having the pipe server and ui write > into the "real" fds? That would seem like a safer and more robust > approach, especially for people with extensions using 3rd party > libraries that may not send all stdio through Mercurial APIs. sys.std* wrapper is useless if underlying libraries use Std-C functions [1]. Also, it could cause trouble if they expect that sys.std* are real file objects. [1]: https://docs.python.org/2.7/library/sys.html#sys.stdin Mercurial extensions should conform to the rule of Mercurial internal API. This patch is the last ditch to avoid corruption of command-server protocol in case they are broken. Regards,
On Sat, 2014-11-15 at 19:34 +0900, Yuya Nishihara wrote: > # HG changeset patch > # User Yuya Nishihara <yuya@tcha.org> > # Date 1416027043 -32400 > # Sat Nov 15 13:50:43 2014 +0900 > # Node ID 15b63d55d18198a1e2621c79419b7b1f66a8875e > # Parent d6e1a0cd0da71ebd777d2f35a52b6390ca3915e8 > cmdserver: protect pipe server streams against corruption caused by direct io These are queued for default, thanks.
Patch
diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py --- a/mercurial/commandserver.py +++ b/mercurial/commandserver.py @@ -7,7 +7,7 @@ from i18n import _ import struct -import os, errno, traceback, SocketServer +import sys, os, errno, traceback, SocketServer import dispatch, encoding, util logfile = None @@ -248,6 +248,29 @@ class server(object): return 0 +def _protectio(ui): + """ duplicates streams and redirect original to null if ui uses stdio """ + ui.flush() + newfiles = [] + nullfd = os.open(os.devnull, os.O_RDWR) + for f, sysf, mode in [(ui.fin, sys.stdin, 'rb'), + (ui.fout, sys.stdout, 'wb')]: + if f is sysf: + newfd = os.dup(f.fileno()) + os.dup2(nullfd, f.fileno()) + f = os.fdopen(newfd, mode) + newfiles.append(f) + os.close(nullfd) + return tuple(newfiles) + +def _restoreio(ui, fin, fout): + """ restores streams from duplicated ones """ + ui.flush() + for f, uif in [(fin, ui.fin), (fout, ui.fout)]: + if f is not uif: + os.dup2(f.fileno(), uif.fileno()) + f.close() + class pipeservice(object): def __init__(self, ui, repo, opts): self.ui = ui @@ -258,8 +281,14 @@ class pipeservice(object): def run(self): ui = self.ui - sv = server(ui, self.repo, ui.fin, ui.fout) - return sv.serve() + # redirect stdio to null device so that broken extensions or in-process + # hooks will never cause corruption of channel protocol. + fin, fout = _protectio(ui) + try: + sv = server(ui, self.repo, fin, fout) + return sv.serve() + finally: + _restoreio(ui, fin, fout) class _requesthandler(SocketServer.StreamRequestHandler): def handle(self): diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t --- a/tests/test-commandserver.t +++ b/tests/test-commandserver.t @@ -492,6 +492,7 @@ check that local configs for the cached foo $ cat <<EOF > dbgui.py + > import os, sys > from mercurial import cmdutil, commands > cmdtable = {} > command = cmdutil.command(cmdtable) @@ -501,6 +502,14 @@ check that local configs for the cached > @command("debugprompt", norepo=True) > def debugprompt(ui): > ui.write("%s\\n" % ui.prompt("prompt:")) + > @command("debugreadstdin", norepo=True) + > def debugreadstdin(ui): + > ui.write("read: %r\n" % sys.stdin.read(1)) + > @command("debugwritestdout", norepo=True) + > def debugwritestdout(ui): + > os.write(1, "low-level stdout fd and\n") + > sys.stdout.write("stdout should be redirected to /dev/null\n") + > sys.stdout.flush() > EOF $ cat <<EOF >> .hg/hgrc > [extensions] @@ -518,10 +527,15 @@ check that local configs for the cached ... runcommand(server, ['debugprompt', '--config', ... 'ui.interactive=True'], ... input=cStringIO.StringIO('5678\n')) + ... runcommand(server, ['debugreadstdin']) + ... runcommand(server, ['debugwritestdout']) *** runcommand debuggetpass --config ui.interactive=True password: 1234 *** runcommand debugprompt --config ui.interactive=True prompt: 5678 + *** runcommand debugreadstdin + read: '' + *** runcommand debugwritestdout run commandserver in commandserver, which is silly but should work:
# HG changeset patch # User Yuya Nishihara <yuya@tcha.org> # Date 1416027043 -32400 # Sat Nov 15 13:50:43 2014 +0900 # Node ID 15b63d55d18198a1e2621c79419b7b1f66a8875e # Parent d6e1a0cd0da71ebd777d2f35a52b6390ca3915e8 cmdserver: protect pipe server streams against corruption caused by direct io Because pipe-mode server uses stdio as IPC channel, other modules should not touch stdio directly and use ui instead. However, this strategy is brittle because several Python functions read and write stdio implicitly. print 'hello' # should use ui.write() # => ch = 'h', size = 1701604463 'ello', data = '\n' This patch adds protection for such mistakes. Both stdio files and low-level file descriptors are redirected to /dev/null while command server uses them.