Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 42 additions & 0 deletions Lib/test/pickletester.py
Original file line number Diff line number Diff line change
Expand Up @@ -1246,6 +1246,48 @@ def test_frame_readline(self):
# 15: . STOP
self.assertEqual(self.loads(pickled), 42)

def test_frame_ends_at_opcode_boundary(self):
# A frame may end exactly between two opcodes; the following opcodes
# are then read from outside the frame.
for pickled in [
b'\x80\x05\x95\x01\x00\x00\x00\x00\x00\x00\x00N.', # FRAME 1, NONE, STOP
b'\x80\x05\x95\x00\x00\x00\x00\x00\x00\x00\x00N.', # empty FRAME, NONE, STOP
]:
with self.subTest(pickled=pickled):
self.assertIsNone(self.loads(pickled))

def test_frame_does_not_straddle_boundary(self):
# An opcode or its argument must not cross a frame boundary
# (PEP 3154). Such a pickle must be rejected rather than silently
# reading past the declared frame length, which would make the
# meaning of the pickle diverge from its pickletools disassembly.
# See gh-154848.
for pickled in [
# FRAME 6; UNICODE argument read by readline() straddles the frame.
b'\x80\x05\x95\x06\x00\x00\x00\x00\x00\x00\x00Vhelloworld\n.',
# FRAME 6; BINUNICODE argument straddles the frame.
b'\x80\x05\x95\x06\x00\x00\x00\x00\x00\x00\x00'
b'X\x0a\x00\x00\x00helloworld.',
# FRAME 3; SHORT_BINBYTES argument straddles the frame.
b'\x80\x05\x95\x03\x00\x00\x00\x00\x00\x00\x00C\x0ahelloworld.',
# FRAME 9; GLOBAL argument (second line) straddles the frame.
b'\x80\x05\x95\x09\x00\x00\x00\x00\x00\x00\x00cbuiltins\nprint\n.',
]:
self.check_unpickling_error(self.truncated_errors, pickled)

def test_nested_frame(self):
# A new frame must not begin before the current one has ended: here
# the outer frame still has data left after the inner frame header.
pickled = (b'\x80\x05\x95\x0c\x00\x00\x00\x00\x00\x00\x00'
b'N\x95\x00\x00\x00\x00\x00\x00\x00\x00NN.')
self.check_unpickling_error(self.truncated_errors, pickled)

# But the inner frame header may lie inside the outer frame as long as
# it exactly consumes it.
pickled = (b'\x80\x05\x95\x0a\x00\x00\x00\x00\x00\x00\x00'
b'N\x95\x00\x00\x00\x00\x00\x00\x00\x00N.')
self.assertIsNone(self.loads(pickled))

def test_compat_unpickle(self):
# xrange(1, 7)
pickled = b'\x80\x02c__builtin__\nxrange\nK\x01K\x07K\x01\x87R.'
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_pickle.py
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ def test_pickler(self):
0) # Write buffer is cleared after every dump().

def test_unpickler(self):
basesize = support.calcobjsize('2P2n3P 2P2n2i5P 2P3n8P2n3i')
basesize = support.calcobjsize('2P2n3P 2P2n2i5P 2P5n8P2n3i')
unpickler = _pickle.Unpickler
P = struct.calcsize('P') # Size of memo table entry.
n = struct.calcsize('n') # Size of mark table entry.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
The :mod:`pickle` C accelerator now enforces frame boundaries when
unpickling, as the pure Python implementation already did. An opcode or its
argument that straddles a frame boundary, or a frame that begins before the
previous one has ended, now raises :exc:`pickle.UnpicklingError` instead of
being silently read across the boundary. This prevents the loaded data from
diverging from the :mod:`pickletools` disassembly of the same pickle.
89 changes: 89 additions & 0 deletions Modules/_pickle.c
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,10 @@ typedef struct UnpicklerObject {
Py_ssize_t input_len;
Py_ssize_t next_read_idx;
Py_ssize_t prefetched_idx; /* index of first prefetched byte */
Py_ssize_t frame_end; /* End of the current frame, or -1 if not in a
frame. While in a frame, input_len is capped
here so reads can't cross it (PEP 3154). */
Py_ssize_t saved_input_len; /* input_len to restore when the frame ends. */

PyObject *read; /* read() method of the input stream. */
PyObject *readinto; /* readinto() method of the input stream. */
Expand Down Expand Up @@ -1250,6 +1254,7 @@ _Unpickler_SetStringInput(UnpicklerObject *self, PyObject *input)
self->input_len = self->buffer.len;
self->next_read_idx = 0;
self->prefetched_idx = self->input_len;
self->frame_end = -1;
return self->input_len;
}

Expand All @@ -1260,6 +1265,30 @@ bad_readline(PickleState *st)
return -1;
}

/* End the current frame, uncapping input_len. The frame must be fully read. */
static void
_Unpickler_EndFrame(UnpicklerObject *self)
{
assert(self->frame_end >= 0);
assert(self->next_read_idx == self->frame_end);
self->input_len = self->saved_input_len;
self->frame_end = -1;
}

/* Reached the end of the current frame. If straddle, the read crosses the
frame boundary (PEP 3154) and fails; otherwise end the frame. */
static int
_Unpickler_LeaveFrame(PickleState *st, UnpicklerObject *self, int straddle)
{
if (straddle) {
PyErr_SetString(st->UnpicklingError,
"pickle exhausted before end of frame");
return -1;
}
_Unpickler_EndFrame(self);
return 0;
}

/* Skip any consumed data that was only prefetched using peek() */
static int
_Unpickler_SkipConsumed(UnpicklerObject *self)
Expand Down Expand Up @@ -1444,6 +1473,19 @@ _Unpickler_ReadImpl(UnpicklerObject *self, PickleState *st, char **s, Py_ssize_t
return -1;
}

if (self->frame_end >= 0) {
if (_Unpickler_LeaveFrame(st, self,
self->next_read_idx < self->frame_end) < 0) {
return -1;
}
/* Frame ended; the read may now be satisfied from the buffer. */
if (n <= self->input_len - self->next_read_idx) {
*s = self->input_buffer + self->next_read_idx;
self->next_read_idx += n;
return n;
}
}

/* This case is handled by the _Unpickler_Read() macro for efficiency */
assert(self->next_read_idx + n > self->input_len);

Expand Down Expand Up @@ -1489,6 +1531,25 @@ _Unpickler_ReadInto(PickleState *state, UnpicklerObject *self, char *buf,
}
}

if (self->frame_end >= 0) {
/* in_buffer > 0 is next_read_idx < frame_end on entry: frame data was
consumed above, so the read crosses the frame boundary. */
if (_Unpickler_LeaveFrame(state, self, in_buffer > 0) < 0) {
return -1;
}
in_buffer = self->input_len - self->next_read_idx;
if (in_buffer > 0) {
Py_ssize_t to_read = Py_MIN(in_buffer, n);
memcpy(buf, self->input_buffer + self->next_read_idx, to_read);
self->next_read_idx += to_read;
buf += to_read;
n -= to_read;
if (n == 0) {
return n;
}
}
}

/* Read from file */
if (!self->read) {
/* We're unpickling memory, this means the input is truncated */
Expand Down Expand Up @@ -1547,6 +1608,7 @@ _Unpickler_Readline(PickleState *state, UnpicklerObject *self, char **result)
{
Py_ssize_t i, num_read;

rescan:
for (i = self->next_read_idx; i < self->input_len; i++) {
if (self->input_buffer[i] == '\n') {
char *line_start = self->input_buffer + self->next_read_idx;
Expand All @@ -1555,6 +1617,14 @@ _Unpickler_Readline(PickleState *state, UnpicklerObject *self, char **result)
return _Unpickler_CopyLine(self, line_start, num_read, result);
}
}
if (self->frame_end >= 0) {
if (_Unpickler_LeaveFrame(state, self,
self->next_read_idx < self->frame_end) < 0) {
return -1;
}
/* Frame ended; continue the line past its end. */
goto rescan;
}
if (!self->read)
return bad_readline(state);

Expand Down Expand Up @@ -1729,6 +1799,8 @@ _Unpickler_New(PyObject *module)
self->input_len = 0;
self->next_read_idx = 0;
self->prefetched_idx = 0;
self->frame_end = -1;
self->saved_input_len = 0;
self->read = NULL;
self->readinto = NULL;
self->readline = NULL;
Expand Down Expand Up @@ -7078,6 +7150,17 @@ load_frame(PickleState *state, UnpicklerObject *self)
if (_Unpickler_Read(self, state, &s, 8) < 0)
return -1;

/* A new frame must not begin before the current one ends (PEP 3154). Its
header may lie at the tail of the current frame if it drains it. */
if (self->frame_end >= 0) {
if (self->next_read_idx < self->frame_end) {
PyErr_SetString(state->UnpicklingError,
"beginning of a new frame before end of current frame");
return -1;
}
_Unpickler_EndFrame(self);
}

frame_len = calc_binsize(s, 8);
if (frame_len < 0) {
PyErr_Format(PyExc_OverflowError,
Expand All @@ -7091,6 +7174,12 @@ load_frame(PickleState *state, UnpicklerObject *self)

/* Rewind to start of frame */
self->next_read_idx -= frame_len;

/* Cap input_len at the frame end so reads can't cross it (PEP 3154);
_Unpickler_EndFrame() restores it when the frame is fully read. */
self->frame_end = self->next_read_idx + frame_len;
self->saved_input_len = self->input_len;
self->input_len = self->frame_end;
return 0;
}

Expand Down
Loading