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

SSLSocket.recv(-1) triggers SystemError #70831

Closed
vadmium opened this issue Mar 26, 2016 · 9 comments
Closed

SSLSocket.recv(-1) triggers SystemError #70831

vadmium opened this issue Mar 26, 2016 · 9 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@vadmium
Copy link
Member

vadmium commented Mar 26, 2016

BPO 26644
Nosy @benjaminp, @vadmium, @serhiy-storchaka
Files
  • ssl-negative.patch
  • ssl-negative.v2.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 = 'https://github.com/vadmium'
    closed_at = <Date 2016-03-27.21:08:01.250>
    created_at = <Date 2016-03-26.07:28:02.792>
    labels = ['extension-modules', 'type-bug']
    title = 'SSLSocket.recv(-1) triggers SystemError'
    updated_at = <Date 2016-03-27.21:08:01.249>
    user = 'https://github.com/vadmium'

    bugs.python.org fields:

    activity = <Date 2016-03-27.21:08:01.249>
    actor = 'martin.panter'
    assignee = 'martin.panter'
    closed = True
    closed_date = <Date 2016-03-27.21:08:01.250>
    closer = 'martin.panter'
    components = ['Extension Modules']
    creation = <Date 2016-03-26.07:28:02.792>
    creator = 'martin.panter'
    dependencies = []
    files = ['42295', '42299']
    hgrepos = []
    issue_num = 26644
    keywords = ['patch']
    message_count = 9.0
    messages = ['262488', '262489', '262497', '262498', '262499', '262500', '262505', '262516', '262536']
    nosy_count = 4.0
    nosy_names = ['benjamin.peterson', 'python-dev', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26644'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @vadmium
    Copy link
    Member Author

    vadmium commented Mar 26, 2016

    SystemError indicates an internal error that is not supposed to be triggerable from Python code. We should probably raise ValueError like plain sockets instead.

    >>> s = create_connection(("python.org", 443))
    >>> s.recv(-1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: negative buffersize in recv
    >>> ss = ssl.wrap_socket(s)
    >>> ss.recv(-1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/proj/python/cpython/Lib/ssl.py", line 910, in recv
        return self.read(buflen)
      File "/home/proj/python/cpython/Lib/ssl.py", line 787, in read
        return self._sslobj.read(len, buffer)
      File "/home/proj/python/cpython/Lib/ssl.py", line 573, in read
        v = self._sslobj.read(len or 1024)
    SystemError: Negative size passed to PyBytes_FromStringAndSize

    @vadmium vadmium added extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Mar 26, 2016
    @vadmium
    Copy link
    Member Author

    vadmium commented Mar 26, 2016

    Proposed patch

    @benjaminp
    Copy link
    Contributor

    Is this what other file-like objects do with negatives sizes?

    @vadmium
    Copy link
    Member Author

    vadmium commented Mar 26, 2016

    Socket objects aren’t exactly file-like. Plain non-SSL sockets don’t even have read() methods.

    I think giving a meaning to recv(-1) would be an (unwanted) new feature, rather than a bug fix. If you want a file-like object linked to a socket, I would suggest using something like the makefile() method instead of adding to the low-level socket object API.

    But to answer your question: no, most file methods treat a negative size as a special request to read until EOF, e.g. read(-1), readline(-1) and readlines(-1) of RawIOBase, BufferedIOBase and TextIOBase. On the other hand, BufferedIOBase.read1(-1) is poorly defined and supported (bpo-23214), but may end up meaning something like “read an arbitrary non-zero chunk with a minimum amount of low-level calls and processing”.

    @benjaminp
    Copy link
    Contributor

    Thanks for the explanation. Your patch lgtm.

    On Sat, Mar 26, 2016, at 15:01, Martin Panter wrote:

    Martin Panter added the comment:

    Socket objects aren’t exactly file-like. Plain non-SSL sockets don’t even
    have read() methods.

    I think giving a meaning to recv(-1) would be an (unwanted) new feature,
    rather than a bug fix. If you want a file-like object linked to a socket,
    I would suggest using something like the makefile() method instead of
    adding to the low-level socket object API.

    But to answer your question: no, most file methods treat a negative size
    as a special request to read until EOF, e.g. read(-1), readline(-1) and
    readlines(-1) of RawIOBase, BufferedIOBase and TextIOBase. On the other
    hand, BufferedIOBase.read1(-1) is poorly defined and supported (Issue
    23214), but may end up meaning something like “read an arbitrary non-zero
    chunk with a minimum amount of low-level calls and processing”.

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue26644\>


    @vadmium
    Copy link
    Member Author

    vadmium commented Mar 26, 2016

    Thanks for the reviews. Here is a patch that avoids breaking read(-1, buffer).

    @serhiy-storchaka
    Copy link
    Member

    LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 27, 2016

    New changeset af92651c22e9 by Martin Panter in branch '3.5':
    Issue bpo-26644: Raise ValueError for negative SSLSocket.recv() and read()
    https://hg.python.org/cpython/rev/af92651c22e9

    New changeset b84d136e0028 by Martin Panter in branch '2.7':
    Issue bpo-26644: Raise ValueError for negative SSLSocket.recv() and read()
    https://hg.python.org/cpython/rev/b84d136e0028

    New changeset 80934ad2356d by Martin Panter in branch 'default':
    Issue bpo-26644: Merge SSL negative read fix from 3.5
    https://hg.python.org/cpython/rev/80934ad2356d

    @vadmium
    Copy link
    Member Author

    vadmium commented Mar 27, 2016

    The Python 2 fix was slightly different, due to the lack of Argument Clinic.

    @vadmium vadmium closed this as completed Mar 27, 2016
    @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
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants