classification
Title: Patch fixing sanity check for ordered fd sequence in _posixsubprocess.c
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Hisham Muhammad, gregory.p.smith, martin.panter, python-dev
Priority: low Keywords: patch

Created on 2015-03-02 16:50 by Hisham Muhammad, last changed 2015-11-16 05:30 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
python-3.4.3-_posixsubprocess.c.fix.patch Hisham Muhammad, 2015-03-02 16:50 Patch for Modules/_posixsubprocess.c fixing _sanity_check_python_fd_sequence
Messages (7)
msg237061 - (view) Author: Hisham Muhammad (Hisham Muhammad) Date: 2015-03-02 16:50
In Modules/_posixsubprocess.c, (the helper module for Lib/subprocess.py) there's a function called _sanity_check_python_fd_sequence which checks, as its comment says, if the fds in the sequence are all positive and sorted.

The check to verify if it is sorted is incorrect (missing the update to the prev_fd variable), and also it is missing a test to see if fd's are repeated (which they shouldn't be, so the test should use <= rather than <). 

The attached patch, written against Python 3.4.3 source code, fixes it.
msg237110 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2015-03-03 07:34
Haha, yes, that description and patch look correct.  Thanks!

Fortunately this bug is low impact as this was just a sanity check and the calling code from subprocess.py was already passing the correct data in.

An ideal regression test: An explicit unittest that calls the _posixsubprocess API with some bad sequences of values in fds_to_keep and uses assertRaises to check for the appropriate ValueError("bad value(s) in fds_to_keep") coming out of it...
msg254699 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2015-11-15 22:36
Hisham, could you sign the Python contributor agreement? https://www.python.org/psf/contrib/contrib-form/

thanks!
msg254701 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-16 00:31
FWIW I think this patch is trivial enough not to need an agreement.
msg254709 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2015-11-16 05:11
heh, you're right.  it's a trivial obvious fix for the mistake in the existing implementation.  writing a test and committing.

the other option would be to get rid of the sanity check entirely or change it not to use the odd "require a sorted list" code.  but i like the sanity check and refactoring it to not require a sorted list within this code path would be complicated.
msg254710 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-11-16 05:29
New changeset 55f7a99a5433 by Gregory P. Smith in branch '3.5':
Fixes #23564: Fix a partially broken sanity check in the _posixsubprocess
https://hg.python.org/cpython/rev/55f7a99a5433

New changeset 97e2a6810f7f by Gregory P. Smith in branch 'default':
Fixes #23564: Fix a partially broken sanity check in the _posixsubprocess
https://hg.python.org/cpython/rev/97e2a6810f7f
msg254711 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2015-11-16 05:30
I didn't bother adding the fix to 3.4, it isn't high value.
History
Date User Action Args
2015-11-16 05:30:44gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg254711

versions: + Python 3.5, Python 3.6
2015-11-16 05:29:54python-devsetnosy: + python-dev
messages: + msg254710
2015-11-16 05:11:16gregory.p.smithsetmessages: + msg254709
2015-11-16 00:31:08martin.pantersetnosy: + martin.panter
messages: + msg254701
2015-11-15 22:36:35gregory.p.smithsetmessages: + msg254699
2015-03-03 07:34:45gregory.p.smithsetpriority: normal -> low
messages: + msg237110

assignee: gregory.p.smith
type: behavior
stage: patch review
2015-03-03 02:40:29ned.deilysetnosy: + gregory.p.smith
2015-03-02 16:50:12Hisham Muhammadcreate