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.

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

_multiprocessing.Connection() doesn't check handle #47571

Closed
vstinner opened this issue Jul 8, 2008 · 19 comments
Closed

_multiprocessing.Connection() doesn't check handle #47571

vstinner opened this issue Jul 8, 2008 · 19 comments
Labels
stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@vstinner
Copy link
Member

vstinner commented Jul 8, 2008

BPO 3321
Nosy @amauryfa, @vstinner, @devdanzin
Files
  • issue_3321.patch
  • 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 = None
    closed_at = <Date 2009-01-19.15:41:40.768>
    created_at = <Date 2008-07-08.22:45:08.890>
    labels = ['library', 'type-crash']
    title = "_multiprocessing.Connection() doesn't check handle"
    updated_at = <Date 2009-01-19.16:05:33.818>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2009-01-19.16:05:33.818>
    actor = 'jnoller'
    assignee = 'jnoller'
    closed = True
    closed_date = <Date 2009-01-19.15:41:40.768>
    closer = 'jnoller'
    components = ['Library (Lib)']
    creation = <Date 2008-07-08.22:45:08.890>
    creator = 'vstinner'
    dependencies = []
    files = ['12797']
    hgrepos = []
    issue_num = 3321
    keywords = ['patch', 'needs review']
    message_count = 19.0
    messages = ['69446', '69448', '70425', '70426', '73158', '73172', '73177', '73192', '80169', '80170', '80174', '80175', '80177', '80178', '80179', '80184', '80185', '80186', '80187']
    nosy_count = 6.0
    nosy_names = ['amaury.forgeotdarc', 'vstinner', 'ocean-city', 'ajaksu2', 'roudkerk', 'jnoller']
    pr_nums = []
    priority = 'critical'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue3321'
    versions = ['Python 2.6']

    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 8, 2008

    _multiprocessing.Connection() allows to use any positive (or nul)
    number has socket handle. If you use an invalid file descriptor,
    poll() method may crash (especially for big positive integer).
    Example:

    >>> import _multiprocessing
    >>> obj = _multiprocessing.Connection(44977608)
    >>> obj.poll()
    Erreur de segmentation (core dumped)

    Fix: use fstat() to make sure that the handle is valid. Attached patch
    implements this feature.

    Another solution would be to reuse code from Modules/selectmodule.c.

    @vstinner vstinner added stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump labels Jul 8, 2008
    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 8, 2008

    Ooops, there is a typo in my last patch: it's "struct stat statbuf;"
    and not "struct stat *statbuf;"! Here is a new version of the patch.

    @jnoller jnoller mannequin self-assigned this Jul 15, 2008
    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jul 30, 2008

    From Victor:
    Ok, here is a patch for test_multiprocessing.py.

    • TestClosedFile.test_open() verify that Connection() rejects closed file
      descriptor
    • TestClosedFile.test_operations() verify that Connection() raises IOError
      for
      operations on closed file

    I don't know if Connection() is a socket or a pipe. Both should be tested.

    @amauryfa
    Copy link
    Member

    I'm quite sure that neither the patch nor the new test make sense on
    Windows. A file handle is not a file descriptor!

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Sep 13, 2008

    Without someone offering some windows help, I won't be able to do a patch.
    My windows fu is lacking.

    @amauryfa
    Copy link
    Member

    Note that Windows does not crash in such cases:

    >>> import socket, _multiprocessing
    >>> obj = _multiprocessing.Connection(44977608)
    >>> obj.poll()
    IOError: [Errno 10038] An operation was attempted on something that is
    not a socket
    
    >>> s = socket.socket()
    >>> obj = _multiprocessing.Connection(s.fileno())
    >>> obj.poll()
    False
    >>> s.close()
    >>> obj.poll()
    IOError: [Errno 10038] An operation was attempted on something that is
    not a socket

    So some "#ifndef MS_WINDOWS" should be enough...

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Sep 13, 2008

    I thought socket handle on BeOS is not file descripter neighter. (I'm
    not sure BeOS is still supported or not)

    Another solution would be to reuse code from Modules/selectmodule.c.

    You mean this code?

    if (v < 0 || v >= FD_SETSIZE) {
    	PyErr_SetString(PyExc_ValueError,
    		    "filedescriptor out of range in select()");
    	goto finally;
    }

    Cygwin's thread is somewhat troublesome, so I'm not sure I can test this
    issue on cygwin but, (I'm now building python --with-threads) if clash
    happens in conn_poll(_multiprocessing/connection.h)'s FD_SET, I think
    this kind of check is needed... (At least safer)

    @ocean-city
    Copy link
    Mannequin

    ocean-city mannequin commented Sep 13, 2008

    I've implemented "another solution". test_open() in
    test_multithreading.patch won't pass though.... It'll raise error in
    conn.poll() not in constructor.

    $ ./dummy.exe b.py
    Traceback (most recent call last):
      File "b.py", line 6, in <module>
        conn.poll()
    IOError: [Errno 9] Bad file descriptor

    test_operations() will pass.

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    Attached is a patch+test for this condition, which is not used if we're
    running on windows.

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    Curse you hard-tabs. Here's the new patch w/ fixed comment

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    Removed raise TestSkip per code review from bpeterson

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    Committed patch as r68768 to python-trunk

    @vstinner
    Copy link
    Member Author

    @jnoller: Hey, you removed my patch! My patch used fstat() in
    Connection constructor, whereas your just check file descriptor bounds
    in the poll() method. And what is the "save" new argument? Is it
    related to this issue?

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    The save was needed for the Py_BLOCK_THREADS call.

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    Ugh, I didn't mean to chuck your original patch, but it also wasn't
    correct for win32

    Additionally, if you close the handle from underneath it, it behaves
    properly:

    >>> obj.poll()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    IOError: [Errno 9] Bad file descriptor
    >>>

    @jnoller jnoller mannequin closed this as completed Jan 19, 2009
    @vstinner
    Copy link
    Member Author

    Why don't you check the file descriptor directly in connection_new()?
    conn->handle is read only and so can't be changed before the call to
    poll(). So other methods will also be "protected" and the error will
    be raised earlier.

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    That's an enhancement - not a bad idea, I just noticed that this issue is
    pretty close to issue http://bugs.python.org/issue3311 as well.

    @vstinner
    Copy link
    Member Author

    jnoller> issue bpo-3311

    Oh, I forgot this issue :-) But the fix doesn't solve bpo-3311, because
    it is disabled on Windows and only protect poll() method.

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Jan 19, 2009

    Oh, I agree - I think we should update 3311 with the enhancement to move
    the check to connection_new

    @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
    Labels
    stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants