Skip to content

Commit 1363a11

Browse files
committed
Reduce comment verbosity in the readinto fix
1 parent 955f1fc commit 1363a11

2 files changed

Lines changed: 9 additions & 26 deletions

File tree

Lib/test/test_pickle.py

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -383,12 +383,8 @@ class CUnpicklerTests(PyUnpicklerTests):
383383
size_overflow_error = (OverflowError, 'exceeds')
384384

385385
def test_readinto_does_not_keep_buffer_alive(self):
386-
# The C unpickler hands readinto() a temporary memoryview over an
387-
# internal buffer that does not outlive the unpickling operation.
388-
# A readinto() implementation that keeps a reference to that view
389-
# must not be able to read or write the buffer after readinto()
390-
# returns: the view is released, so using it raises ValueError
391-
# rather than accessing freed memory.
386+
# A readinto() that retains the memoryview it is handed must not be
387+
# able to access the buffer after readinto() returns (gh-151046).
392388
stashed = []
393389

394390
class StashingFile:

Modules/_pickle.c

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1283,13 +1283,9 @@ _Unpickler_SkipConsumed(UnpicklerObject *self)
12831283

12841284
static const Py_ssize_t READ_WHOLE_LINE = -1;
12851285

1286-
/* Release the temporary memoryview that was handed to a readinto() call and
1287-
drop our reference to it. Releasing it severs the link between the view and
1288-
the underlying buffer, so a reference retained by readinto() can no longer be
1289-
used to read or write the (soon to be freed) buffer. Returns 0 on success,
1290-
-1 if the view could not be released (for example because readinto() exported
1291-
the buffer and still holds it); in that case an exception is set. Any
1292-
exception already pending when this is called is preserved. */
1286+
/* Release the temporary memoryview handed to readinto() and drop our reference,
1287+
so a view retained by readinto() can no longer reach the underlying buffer.
1288+
Returns -1 if release() failed. Any pending exception is preserved. */
12931289
static int
12941290
_Unpickler_ReleaseBufObj(PyObject *buf_obj)
12951291
{
@@ -1298,7 +1294,6 @@ _Unpickler_ReleaseBufObj(PyObject *buf_obj)
12981294
int err = (res == NULL) ? -1 : 0;
12991295
Py_XDECREF(res);
13001296
if (exc != NULL) {
1301-
/* Keep the original error; ignore any error from release(). */
13021297
if (err < 0) {
13031298
PyErr_Clear();
13041299
}
@@ -1344,22 +1339,14 @@ _Unpickler_ReadIntoFromFile(PickleState *state, UnpicklerObject *self, char *buf
13441339
return n;
13451340
}
13461341

1347-
/* Call readinto() into user buffer.
1348-
1349-
buf points into a temporary buffer (e.g. a bytes object on the
1350-
unpickler stack) that does not outlive this unpickling operation. We
1351-
wrap it in a memoryview only so we can hand it to readinto(); a buggy or
1352-
hostile readinto() implementation may keep a reference to that view.
1353-
Release the view as soon as readinto() returns so that any surviving
1354-
reference can no longer dereference buf: using it then raises a clean
1355-
Python exception instead of accessing freed memory. */
1342+
/* buf is a temporary buffer; we wrap it in a memoryview only to pass it to
1343+
readinto(). Release the view once readinto() returns, so a view it kept
1344+
a reference to cannot be used to access buf after it is freed. */
13561345
PyObject *buf_obj = PyMemoryView_FromMemory(buf, n, PyBUF_WRITE);
13571346
if (buf_obj == NULL) {
13581347
return -1;
13591348
}
1360-
/* Keep our own reference across the call (PyObject_CallOneArg does not
1361-
steal it) so that we can release the view afterwards regardless of what
1362-
readinto() does with the object it receives. */
1349+
/* PyObject_CallOneArg does not steal buf_obj, so we can release it below. */
13631350
PyObject *read_size_obj = PyObject_CallOneArg(self->readinto, buf_obj);
13641351
if (read_size_obj == NULL) {
13651352
_Unpickler_ReleaseBufObj(buf_obj);

0 commit comments

Comments
 (0)