gh-154848: Enforce frame boundaries in the C unpickler (GH-154893)

The C unpickler did not enforce PEP 3154 frame boundaries: an opcode or its
argument could straddle a frame, and a new frame could begin before the
previous one ended.  Such reads now raise UnpicklingError, as in the pure
Python implementation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index 9ba498ce8f575deb4f4a04ffa99d9106247a08f2..83a71b7efedd379718662efbb2e5c38436e00b73 100644 (file)
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -1246,6 +1246,48 @@ class AbstractUnpickleTests:
         #   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\x04\x95\x01\x00\x00\x00\x00\x00\x00\x00N.',  # FRAME 1, NONE, STOP
+            b'\x80\x04\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\x04\x95\x06\x00\x00\x00\x00\x00\x00\x00Vhelloworld\n.',
+            # FRAME 6; BINUNICODE argument straddles the frame.
+            b'\x80\x04\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\x04\x95\x03\x00\x00\x00\x00\x00\x00\x00C\x0ahelloworld.',
+            # FRAME 9; GLOBAL argument (second line) straddles the frame.
+            b'\x80\x04\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\x04\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\x04\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.'

diff --git a/Lib/test/test_pickle.py b/Lib/test/test_pickle.py
index 48375cf459ea0b15cd33dc613b09392984718fcf..21f45339e8a344226aa763620bef73b42c3a4b50 100644 (file)
--- a/Lib/test/test_pickle.py
+++ b/Lib/test/test_pickle.py
@@ -524,7 +524,7 @@ if has_c_implementation:
                 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.

diff --git a/Misc/NEWS.d/next/Library/2026-07-29-18-30-00.gh-issue-154848.Fr4meB.rst b/Misc/NEWS.d/next/Library/2026-07-29-18-30-00.gh-issue-154848.Fr4meB.rst
new file mode 100644 (file)
index 0000000..02ca6c6
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2026-07-29-18-30-00.gh-issue-154848.Fr4meB.rst
@@ -0,0 +1,6 @@
+The :mod:`pickle` C accelerator now enforces frame boundaries when
+unpickling, as the pure Python implementation already did.  An 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.

diff --git a/Modules/_pickle.c b/Modules/_pickle.c
index 034dd06b6ab4609f75f1ed32f002cab3c1306c64..90200a4379319d9fdbaec3d800502571993de8cb 100644 (file)
--- a/Modules/_pickle.c
+++ b/Modules/_pickle.c
@@ -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. */
@@ -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;
 }
 
@@ -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)
@@ -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);
 
@@ -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 */
@@ -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;
@@ -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);
 
@@ -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;
@@ -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,
@@ -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;
 }