Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Patch fixing sanity check for ordered fd sequence in _posixsubprocess.c #67752

Closed
HishamMuhammad mannequin opened this issue Mar 2, 2015 · 7 comments
Closed

Patch fixing sanity check for ordered fd sequence in _posixsubprocess.c #67752

HishamMuhammad mannequin opened this issue Mar 2, 2015 · 7 comments
Assignees
@gpshead
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@HishamMuhammad
Copy link
Mannequin

HishamMuhammad mannequin commented Mar 2, 2015

BPO 23564
Nosy @gpshead, @vadmium
Files
  • python-3.4.3-_posixsubprocess.c.fix.patch: Patch for Modules/_posixsubprocess.c fixing _sanity_check_python_fd_sequence
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/gpshead'
closed_at = <Date 2015-11-16.05:30:44.375>
created_at = <Date 2015-03-02.16:50:12.518>
labels = ['extension-modules', 'type-bug']
title = 'Patch fixing sanity check for ordered fd sequence in _posixsubprocess.c'
updated_at = <Date 2015-11-16.05:30:44.357>
user = 'https://bugs.python.org/HishamMuhammad'

    bugs.python.org fields:

    activity = <Date 2015-11-16.05:30:44.357>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2015-11-16.05:30:44.375>
closer = 'gregory.p.smith'
components = ['Extension Modules']
creation = <Date 2015-03-02.16:50:12.518>
creator = 'Hisham Muhammad'
dependencies = []
files = ['38302']
hgrepos = []
issue_num = 23564
keywords = ['patch']
message_count = 7.0
messages = ['237061', '237110', '254699', '254701', '254709', '254710', '254711']
nosy_count = 4.0
nosy_names = ['gregory.p.smith', 'python-dev', 'martin.panter', 'Hisham Muhammad']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23564'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']
    @HishamMuhammad
    Copy link
    Mannequin Author

    HishamMuhammad mannequin commented Mar 2, 2015

    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.

    @HishamMuhammad HishamMuhammad mannequin added the extension-modules C modules in the Modules dir label Mar 2, 2015
    @gpshead
    Copy link
    Member

    gpshead commented Mar 3, 2015

    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...

    @gpshead gpshead self-assigned this Mar 3, 2015
    @gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Mar 3, 2015
    @gpshead
    Copy link
    Member

    gpshead commented Nov 15, 2015

    Hisham, could you sign the Python contributor agreement? https://www.python.org/psf/contrib/contrib-form/

    thanks!

    @vadmium
    Copy link
    Member

    vadmium commented Nov 16, 2015

    FWIW I think this patch is trivial enough not to need an agreement.

    @gpshead
    Copy link
    Member

    gpshead commented Nov 16, 2015

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 16, 2015

    New changeset 55f7a99a5433 by Gregory P. Smith in branch '3.5':
    Fixes bpo-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 bpo-23564: Fix a partially broken sanity check in the _posixsubprocess
    https://hg.python.org/cpython/rev/97e2a6810f7f

    @gpshead
    Copy link
    Member

    gpshead commented Nov 16, 2015

    I didn't bother adding the fix to 3.4, it isn't high value.

    @gpshead gpshead closed this as completed Nov 16, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @gpshead gpshead

    Labels
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @gpshead @vadmium