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

bpo-28557: error message for bad raw readinto #7496

Merged
merged 3 commits into from
Jun 15, 2020
Merged

bpo-28557: error message for bad raw readinto #7496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b8468e
bpo-28557: error message for bad raw readinto
davidszotten Jun 7, 2018
69063e4
fix indentation
benjaminp Jun 15, 2020
3864512
improve error message; it didn't return if it raised an exception
benjaminp Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Lib/test/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,6 +1587,22 @@ def test_args_error(self):
with self.assertRaisesRegex(TypeError, "BufferedReader"):
self.tp(io.BytesIO(), 1024, 1024, 1024)

def test_bad_readinto_value(self):
rawio = io.BufferedReader(io.BytesIO(b"12"))
rawio.readinto = lambda buf: -1
bufio = self.tp(rawio)
with self.assertRaises(OSError) as cm:
bufio.readline()
self.assertIsNone(cm.exception.__cause__)

def test_bad_readinto_type(self):
rawio = io.BufferedReader(io.BytesIO(b"12"))
rawio.readinto = lambda buf: b''
bufio = self.tp(rawio)
with self.assertRaises(OSError) as cm:
bufio.readline()
self.assertIsInstance(cm.exception.__cause__, TypeError)


class PyBufferedReaderTest(BufferedReaderTest):
tp = pyio.BufferedReader
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1677,6 +1677,7 @@ Péter Szabó
John Szakmeister
Piotr Szczepaniak
Amir Szekely
David Szotten
Maciej Szulik
Joel Taddei
Arfrever Frehtes Taifersar Arahesis
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2018-06-07-22-04-01.bpo-28557.ViNJnK.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve the error message for a misbehaving ``rawio.readinto``
9 changes: 9 additions & 0 deletions Modules/_io/bufferedio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1483,6 +1483,15 @@ _bufferedreader_raw_read(buffered *self, char *start, Py_ssize_t len)
}
n = PyNumber_AsSsize_t(res, PyExc_ValueError);
Py_DECREF(res);

if (n == -1 && PyErr_Occurred()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (n == -1 && PyErr_Occurred()) {
if (n == -1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't quite understand why, but removing that check i get a segfault

here's the bottom of the stack from lldb:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001001542f5 python.exe`_PyErr_FormatVFromCause [inlined] _Py_DECREF(op=0x0000000000000000) at object.h:441:9 [opt]
    frame #1: 0x00000001001542f5 python.exe`_PyErr_FormatVFromCause(tstate=0x0000000100506680, exception=0x0000000100292fc0, format="raw readinto() returned invalid value", vargs=0x00007ffeefbf9a40) at errors.c:586 [opt]
  * frame #2: 0x0000000100154460 python.exe`_PyErr_FormatFromCause(exception=<unavailable>, format=<unavailable>) at errors.c:626:5 [opt]
    frame #3: 0x00000001001e8508 python.exe`_bufferedreader_raw_read(self=<unavailable>, start=<unavailable>, len=8192) at bufferedio.c:1488:9 [opt]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hang on, i think i get it now. the point is that n is the return value of the (user-defined) readinto which means it can sometimes be -1 even without PyErr_Occurred. this is then caught in the next if block (and becomes "raw readinto() returned invalid length [..]")

the whole point of this pr is to help users debug bad readinto implementations by providing more granular error messages

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sorry, this was a mistake :/

_PyErr_FormatFromCause(
PyExc_OSError,
"raw readinto() failed"
);
return -1;
}

if (n < 0 || n > len) {
PyErr_Format(PyExc_OSError,
"raw readinto() returned invalid length %zd "
Expand Down