| Submitter | Bryan O'Sullivan |
|---|---|
| Date | Jan. 5, 2016, 5:56 a.m. |
| Message ID | <09ce74e1f92f8838bbcd.1451973370@bryano-mbp.local> |
| Download | mbox | patch |
| Permalink | /patch/12520/ |
| State | Changes Requested |
| Delegated to: | Yuya Nishihara |
| Headers | show |
Comments
On Mon, 04 Jan 2016 21:56:10 -0800, Bryan O'Sullivan wrote: > # HG changeset patch > # User Bryan O'Sullivan <bos@serpentine.com> > # Date 1451973286 28800 > # Mon Jan 04 21:54:46 2016 -0800 > # Node ID 09ce74e1f92f8838bbcd2ac1309519f0448ffbed > # Parent 3838e999a656ccd5644fadac75ed91f15ad22854 > mail: switch from os to subprocess module when invoking sendmail > > This gets rid of some ugly string mangling; avoids a potentially > unsafe trip through the shell; and (bonus!) reports the correct > exit code if sendmail fails. > > diff --git a/mercurial/mail.py b/mercurial/mail.py > --- a/mercurial/mail.py > +++ b/mercurial/mail.py > @@ -12,6 +12,7 @@ import os > import quopri > import smtplib > import socket > +import subprocess > import sys > import time > > @@ -161,12 +162,13 @@ def _smtp(ui): > def _sendmail(ui, sender, recipients, msg): > '''send mail using sendmail.''' > program = ui.config('email', 'method', 'smtp') > - cmdline = '%s -f %s %s' % (program, util.email(sender), > - ' '.join(map(util.email, recipients))) > - ui.note(_('sending mail: %s\n') % cmdline) > - fp = util.popen(cmdline, 'w') > - fp.write(msg) > - ret = fp.close() > + cmdline = ([program, '-f', util.email(sender)] + > + [util.email(r) for r in recipients]) > + ui.note(_('sending mail: %s\n') % ' '.join(util.shellquote(c) > + for c in cmdline)) > + p = subprocess.Popen(cmdline, stdin=subprocess.PIPE) Can we assume that email.method is a single executable path? I don't oppose to this change, but I'm asking it because ui.editor can be a shell command with options.
On Wed, 2016-01-06 at 00:11 +0900, Yuya Nishihara wrote: > On Mon, 04 Jan 2016 21:56:10 -0800, Bryan O'Sullivan wrote: > > # HG changeset patch > > # User Bryan O'Sullivan <bos@serpentine.com> > > # Date 1451973286 28800 > > # Mon Jan 04 21:54:46 2016 -0800 > > # Node ID 09ce74e1f92f8838bbcd2ac1309519f0448ffbed > > # Parent 3838e999a656ccd5644fadac75ed91f15ad22854 > > mail: switch from os to subprocess module when invoking sendmail Except it's actually switching from -util- to subprocess. Util is where we put our os abstractions and this is one so you should either fix util.popen to be sensible or add another method there. > avoids a potentially unsafe trip through the shell; This is not part of our threat model. As a command line tool, we assume you can already run arbitrary shell commands with your existing privileges and if you want to run arbitrary shell commands -through- hg, we're more than happy to help you in a bunch of different ways. The shell is our friend. > > and (bonus!) reports the correct > > exit code if sendmail fails. The weird number is 256 * exit code. Linux et al roll the exit code into the wait(2) return code for unpacking with WEXITSTATUS(). -- Mathematics is the supreme nostalgia of our time.
Patch
diff --git a/mercurial/mail.py b/mercurial/mail.py --- a/mercurial/mail.py +++ b/mercurial/mail.py @@ -12,6 +12,7 @@ import os import quopri import smtplib import socket +import subprocess import sys import time @@ -161,12 +162,13 @@ def _smtp(ui): def _sendmail(ui, sender, recipients, msg): '''send mail using sendmail.''' program = ui.config('email', 'method', 'smtp') - cmdline = '%s -f %s %s' % (program, util.email(sender), - ' '.join(map(util.email, recipients))) - ui.note(_('sending mail: %s\n') % cmdline) - fp = util.popen(cmdline, 'w') - fp.write(msg) - ret = fp.close() + cmdline = ([program, '-f', util.email(sender)] + + [util.email(r) for r in recipients]) + ui.note(_('sending mail: %s\n') % ' '.join(util.shellquote(c) + for c in cmdline)) + p = subprocess.Popen(cmdline, stdin=subprocess.PIPE) + p.communicate(msg) + ret = p.wait() if ret: raise error.Abort('%s %s' % ( os.path.basename(program.split(None, 1)[0]), diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t --- a/tests/test-patchbomb.t +++ b/tests/test-patchbomb.t @@ -2860,7 +2860,7 @@ single rev, and introduce a deliberate f sending [PATCH] test ... sending mail: $TESTTMP/t2/pretendmail.sh -f test foo - abort: pretendmail.sh exited with status 19968 + abort: pretendmail.sh exited with status 78 [255] Test pull url header =================================