classification
Title: SSLSocket.recv(-1) triggers SystemError
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: martin.panter Nosy List: benjamin.peterson, martin.panter, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-03-26 07:28 by martin.panter, last changed 2016-03-27 21:08 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
ssl-negative.patch martin.panter, 2016-03-26 08:03 review
ssl-negative.v2.patch martin.panter, 2016-03-26 23:07 review
Messages (9)
msg262488 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-26 07:28
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
msg262489 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-26 08:03
Proposed patch
msg262497 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2016-03-26 20:47
Is this what other file-like objects do with negatives sizes?
msg262498 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-26 22:01
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”.
msg262499 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2016-03-26 22:05
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>
> _______________________________________
msg262500 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-26 23:07
Thanks for the reviews. Here is a patch that avoids breaking read(-1, buffer).
msg262505 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-03-27 04:04
LGTM.
msg262516 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-03-27 10:41
New changeset af92651c22e9 by Martin Panter in branch '3.5':
Issue #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 #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 #26644: Merge SSL negative read fix from 3.5
https://hg.python.org/cpython/rev/80934ad2356d
msg262536 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-27 21:08
The Python 2 fix was slightly different, due to the lack of Argument Clinic.
History
Date User Action Args
2016-03-27 21:08:01martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg262536

stage: commit review -> resolved
2016-03-27 10:41:34python-devsetnosy: + python-dev
messages: + msg262516
2016-03-27 04:04:30serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg262505

assignee: martin.panter
stage: patch review -> commit review
2016-03-26 23:07:13martin.pantersetfiles: + ssl-negative.v2.patch

messages: + msg262500
2016-03-26 22:05:41benjamin.petersonsetmessages: + msg262499
2016-03-26 22:01:55martin.pantersetmessages: + msg262498
2016-03-26 20:47:05benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg262497
2016-03-26 08:03:15martin.pantersetfiles: + ssl-negative.patch
keywords: + patch
messages: + msg262489

stage: patch review
2016-03-26 07:28:02martin.pantercreate