Skip to content

Commit 955f1fc

Browse files
committed
gh-151046: Fix use-after-free in _Unpickler_ReadIntoFromFile
When unpickling from a file-like object that provides readinto(), the C Unpickler handed it a temporary memoryview over an internal buffer and never released it. A readinto() implementation that kept a reference to the view could read or write the buffer after it had been freed, a use-after-free reachable from pure Python. Keep an owned reference across the call and release the memoryview as soon as readinto() returns, so a surviving reference raises ValueError instead of dereferencing freed memory. Add a regression test.
1 parent 5755d0f commit 955f1fc

3 files changed

Lines changed: 97 additions & 2 deletions

File tree

Lib/test/test_pickle.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,53 @@ class CUnpicklerTests(PyUnpicklerTests):
382382
truncated_data_error = (pickle.UnpicklingError, 'truncated')
383383
size_overflow_error = (OverflowError, 'exceeds')
384384

385+
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.
392+
stashed = []
393+
394+
class StashingFile:
395+
def __init__(self, data):
396+
self._data = memoryview(data)
397+
self._pos = 0
398+
399+
def read(self, n=-1):
400+
if n is None or n < 0:
401+
chunk = self._data[self._pos:]
402+
else:
403+
chunk = self._data[self._pos:self._pos + n]
404+
self._pos += len(chunk)
405+
return bytes(chunk)
406+
407+
def readline(self):
408+
return self.read(-1)
409+
410+
def readinto(self, view):
411+
stashed.append(view)
412+
n = min(len(view), len(self._data) - self._pos)
413+
view[:n] = self._data[self._pos:self._pos + n]
414+
self._pos += n
415+
return n
416+
417+
# A large bytes object forces the file-read (readinto) path.
418+
payload = b'spam' * 100_000
419+
data = pickle.dumps(payload, protocol=4)
420+
obj = self.unpickler(StashingFile(data)).load()
421+
self.assertEqual(obj, payload)
422+
423+
self.assertTrue(stashed)
424+
for view in stashed:
425+
with self.assertRaises(ValueError):
426+
view[0]
427+
with self.assertRaises(ValueError):
428+
view[0] = 0
429+
with self.assertRaises(ValueError):
430+
bytes(view)
431+
385432
class CPicklingErrorTests(PyPicklingErrorTests):
386433
pickler = _pickle.Pickler
387434

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
Fix a use-after-free in the C implementation of :mod:`pickle`. When
2+
unpickling from a file-like object that provides ``readinto()``, the
3+
:class:`~pickle.Unpickler` passed it a temporary :class:`memoryview` over an
4+
internal buffer. A ``readinto()`` implementation that kept a reference to that
5+
view could use it to read or write the buffer after it had been freed. The
6+
view is now released as soon as ``readinto()`` returns, so a surviving
7+
reference raises :exc:`ValueError` instead of accessing freed memory.

Modules/_pickle.c

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1283,6 +1283,32 @@ _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. */
1293+
static int
1294+
_Unpickler_ReleaseBufObj(PyObject *buf_obj)
1295+
{
1296+
PyObject *exc = PyErr_GetRaisedException();
1297+
PyObject *res = PyObject_CallMethodNoArgs(buf_obj, &_Py_ID(release));
1298+
int err = (res == NULL) ? -1 : 0;
1299+
Py_XDECREF(res);
1300+
if (exc != NULL) {
1301+
/* Keep the original error; ignore any error from release(). */
1302+
if (err < 0) {
1303+
PyErr_Clear();
1304+
}
1305+
PyErr_SetRaisedException(exc);
1306+
err = 0;
1307+
}
1308+
Py_DECREF(buf_obj);
1309+
return err;
1310+
}
1311+
12861312
/* Don't call it directly: use _Unpickler_ReadInto() */
12871313
static Py_ssize_t
12881314
_Unpickler_ReadIntoFromFile(PickleState *state, UnpicklerObject *self, char *buf,
@@ -1318,17 +1344,32 @@ _Unpickler_ReadIntoFromFile(PickleState *state, UnpicklerObject *self, char *buf
13181344
return n;
13191345
}
13201346

1321-
/* Call readinto() into user buffer */
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. */
13221356
PyObject *buf_obj = PyMemoryView_FromMemory(buf, n, PyBUF_WRITE);
13231357
if (buf_obj == NULL) {
13241358
return -1;
13251359
}
1326-
PyObject *read_size_obj = _Pickle_FastCall(self->readinto, buf_obj);
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. */
1363+
PyObject *read_size_obj = PyObject_CallOneArg(self->readinto, buf_obj);
13271364
if (read_size_obj == NULL) {
1365+
_Unpickler_ReleaseBufObj(buf_obj);
13281366
return -1;
13291367
}
13301368
Py_ssize_t read_size = PyLong_AsSsize_t(read_size_obj);
13311369
Py_DECREF(read_size_obj);
1370+
if (_Unpickler_ReleaseBufObj(buf_obj) < 0) {
1371+
return -1;
1372+
}
13321373

13331374
if (read_size < 0) {
13341375
if (!PyErr_Occurred()) {

0 commit comments

Comments
 (0)