[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Christoph Neuroth]

Christoph Neuroth  added the comment:

As recommended by eric.smith on #7950, I'd like to suggest further extending 
the documentation to include a security warning about (quite easily) possible 
code injection bugs when using the shell=True parameter (similar to other 
places in the documentation).

--
nosy: +christoph.neuroth

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

Thanks for catching that.  Fixed r77987 r77988.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

One problem with the 3.x versions: the raw_input() should be input().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Thanks to all for the copious feedback & suggestions, and R. David Murray for 
his superior docs writing skills!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

Merged as part of r77961 (2.6), r77962 (py3k), and r77963 (3.1).  Print fixed 
for py3.

--
stage: patch review -> committed/rejected
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Eric Smith]

Eric Smith  added the comment:

When merging to py3k, don't forget to modify the print statement to be a 
function.

--
status: pending -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Nick Coghlan]

Nick Coghlan  added the comment:

Committed for 2.7 as r77959.

Still needs to be merged to the other branches.

--
resolution:  -> accepted
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Nick Coghlan]

Changes by Nick Coghlan :


Removed file: http://bugs.python.org/file16098/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Eric Smith]

Eric Smith  added the comment:

I think this is an improvement to the existing docs, and should be committed.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file16109/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Okay, now if this could just get dev review...

--
Added file: http://bugs.python.org/file16128/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Eric Smith]

Eric Smith  added the comment:

Now you need to put the import of subprocess back in!

Otherwise it looks good to me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file16108/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Added file: http://bugs.python.org/file16109/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

The change to echo is an excellent improvement.  You forgot to change 'python' 
to 'echo' in the following paragraph, though. You are also correct about 
/bin/sh vs sh, my bad.  And I was even looking at the source code when I wrote 
that...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file16101/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

This version takes Murray's most recent draft, applies some minor tweaks from 
my prior patch, and has the "python -c" etc. changed to "echo '$MONEY'" so the 
sh -c comment is completely unambiguous (and it's a simpler example generally).

Whether the subprocess.Popen() call should be demonstrated in the code snippet 
remains an open issue.

--
Added file: http://bugs.python.org/file16108/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

I'm happy to delete the two sentences about quoting.

As for -c, you are so right that it is cryptic.  In the new version of the 
patch I've changed the sentence to be as precise as possible, but I'm not at 
all convinced that it is less confusing.  This is why I think the API should be 
changed.

--
Added file: http://bugs.python.org/file16107/subprocess-doc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Antoine Pitrou]

Antoine Pitrou  added the comment:

The following sentences look like a distraction to me:

« Additional quoting may be required because the entire string is a Python 
string.  It may be useful to use a Python raw string in complex cases. »

It is true of every string literal and is not specific to the subprocess module.

Also, I don't understand what the reference to "-c" is about. subprocess isn't 
only for executing Python interpreters. Or perhaps you are talking about "sh 
-c", but it is quite cryptic.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

By the way, I've been wanting the Popen docs improved for a long time but never 
got around to it, so thanks for pushing for this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

Changes by R. David Murray :


Removed file: http://bugs.python.org/file16100/subprocess-doc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

My placement of the note was carefully considered.  It is discussing the 
shell=False case and IMO belongs after that paragraph.  I understand now your 
concern about the note I omitted...and again I think this is a bug in the Popen 
API.  If shell=False, I think Popen should generate an error if args is a 
string.  Absent that fix, I've reworded the existing sentence along the lines 
you suggest, but using positive language rather than negative.  I've 
incorporated your fix for Eric's bug report, but I haven't added in the 
explicit references to the strings in the example.  IMO those are obvious and 
don't need repeating, but if others think it is clearer to add them, I'm fine 
with that.

As for subprocess, IMO there's no need to show the subprocess call.  The shlex 
result shows the list that would go into the subprocess call, and I think 
that's sufficient.  What do others think?

I've updated my version of the patch.

--
versions: +Python 2.7, Python 3.1
Added file: http://bugs.python.org/file16102/subprocess-doc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file16095/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file15033/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Counterpatch incorporating R. David Murray's succinctness improvements while 
retaining correct positioning of the first note, managing to incorporate the 
3rd note not present in Mr. Murray's, and including more precise wording to 
address the problem Eric Smith pointed out.

--
Added file: http://bugs.python.org/file16101/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Eric Smith]

Eric Smith  added the comment:

>From David's patch:

   Note in particular that options and their arguments go in separate list
   elements, while arguments that need quoting when used in the shell
   (such as filenames containing spaces or the python command shown
   above) are single list elements.

It's not always true that options and their arguments should be in separate 
elements. For example:
['/bin/ls', '-l',  '--block-size=1024']
I understand the sentiment, but if we're trying to give guidance that people 
can follow without understanding the underlying issue, then they should be 
correct all the time.

Also, the code snippet includes subprocess without using it.

--
nosy: +eric.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

Woops, spotted a word I left out.  Fixed.

--
Added file: http://bugs.python.org/file16100/subprocess-doc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

Changes by R. David Murray :


Removed file: http://bugs.python.org/file16099/subprocess-doc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

Hmm.  Somehow the patch got lost.  Let's try again.

--
Added file: http://bugs.python.org/file16099/subprocess-doc.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[R. David Murray]

R. David Murray  added the comment:

I like the idea of pointing out that shlex can be used to determine exactly 
what to pass to subprocess, but I agree that the proposed patch is too wordy 
(and still too much in a negative voice).

Here is an alternate simpler patch.

Note that while I have also clarified the last sentence in the shell=True case, 
I think that this is in fact a bug in the Popen API.  As far as I can tell the 
ability to pass arguments to the shell after the -c is useless, and I think 
Popen should instead raise a ValueError if passed a sequence when shell is 
True.  But that is a different bug report

--
nosy: +r.david.murray
priority:  -> normal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Well, the same basic example is used for cohesiveness, but the issue/pitfall 
being highlighted in each note is distinct. But you have a point about shlex 
being pointed out twice, so here's a version with that redundancy excised.

--
Added file: http://bugs.python.org/file16098/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Antoine Pitrou]

Antoine Pitrou  added the comment:

I still think this is much too verbose. The second note looks redundant with 
the first one and the third one.
Remember, the notes you are adding may be useful, but they'll also make reading 
the whole page more tedious.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file16094/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file14770/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Gonna have to disagree about the raw_input(), because the escaping involved 
would complicate the example and could be distracting/confusing.
Rest of Brian's suggestions taken into account.

--
Added file: http://bugs.python.org/file16095/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Brian Curtin]

Brian Curtin  added the comment:

The raw_input() doesn't provide anything. I'd just drop that and pass the 
string directly to shlex.split.

"Do not put an argument-taking option together with its argument as a single 
item in the *args* list"
-- Something like "Argument-taking options should not be grouped with their 
arguments in the *args* list." may be a cleaner read, and avoids the big scary 
"do not".

"Because that is incorrect"
-- Maybe something like "shlex.split would suggest the following" instead?

--
keywords: +needs review
nosy: +brian.curtin
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Okay; new, hopefully better, draft. Feedback?

--
versions:  -Python 3.1
Added file: http://bugs.python.org/file16094/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Working on a more concise new draft...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Terry J. Reedy]

Terry J. Reedy  added the comment:

PS. I only looked at the style of the patch. Once that is clearer, someone who 
knows more than me needs to review it for content correctness.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Terry J. Reedy]

Terry J. Reedy  added the comment:

The note starts out "Do not (do x) unless you're not (doing y)". This is 
confusing. I believe this means "If you are (doing y), you may (do x)". If so, 
please write it so. If not, please write what you do mean.

After thinking about it, "Only (do x) if you are (doing y)" might be a better 
rewrite. Or "You may (do x) only if you are (doing y)"

Change "be mindful of escaping" to the more direct "escape". Perhaps give a 
list of possible special characters.

I agree that this seems a bit verbose for reference documentation. Perhaps 
there should be a separate PyWiki entry or HowTo doc. That could, for instance, 
list the special characters for various shells if they are not all the same.

--
nosy: +tjreedy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Antoine Pitrou]

Antoine Pitrou  added the comment:

This is a particularly verbose patch for the information it is trying to convey.

--
nosy: +pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Nick Coghlan]

Changes by Nick Coghlan :


--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


Removed file: http://bugs.python.org/file14817/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Chris Rebert  added the comment:

Ok, changed to "note" directives instead of "warning"s. Anything else
that keeps this from being applied?

--
Added file: http://bugs.python.org/file15033/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Benjamin Peterson]

Benjamin Peterson  added the comment:

We are trying to cut down on the number of "warning" directives in the
docs; I think a "note" directive would be appropriate for yours.

--
nosy: +benjamin.peterson

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


--
versions: +Python 3.1, Python 3.2
Added file: http://bugs.python.org/file14817/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

Changes by Chris Rebert :


--
type:  -> feature request

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6760] patch to subprocess docs to better explain Popen's 'args' argument

[Chris Rebert]

New submission from Chris Rebert :

>From what I've seen on several c.l.p threads, some people have a tough
time figuring the correct 'args' argument to subprocess.Popen's
constructor. In an effort to cut down on such discussions in the future,
I've written the attached docs patch to better explain the subject.

I'm not an rst/Sphinx guru and thus was unable to actually test the
patch, so there might be some (hopefully minor) formatting errors.

--
assignee: georg.brandl
components: Documentation
files: subprocess.rst.patch
keywords: patch
messages: 91859
nosy: cvrebert, georg.brandl
severity: normal
status: open
title: patch to subprocess docs to better explain Popen's 'args' argument
versions: Python 2.6
Added file: http://bugs.python.org/file14770/subprocess.rst.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com