Patchwork [4,of,4] mail: switch from os to subprocess module when invoking sendmail

login
register
mail settings
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

Bryan O'Sullivan - Jan. 5, 2016, 5:56 a.m.
# 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.
Yuya Nishihara - Jan. 5, 2016, 3:11 p.m.
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.
Matt Mackall - Jan. 11, 2016, 10:54 p.m.
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
 =================================