bpo-30249: Improve struct.unpack_from() error messages (GH-6059)

5 files changed, 67 insertions, 15 deletions

diff --git a/Doc/library/struct.rst b/Doc/library/struct.rst
index 2d0866c..d6a3cb7 100644
--- a/Doc/library/struct.rst
+++ b/Doc/library/struct.rst
@@ -74,8 +74,8 @@ The module defines the following exception and functions:
 
    Unpack from *buffer* starting at position *offset*, according to the format
    string *format*.  The result is a tuple even if it contains exactly one
-   item.  The buffer's size in bytes, minus *offset*, must be at least
-   the size required by the format, as reflected by :func:`calcsize`.
+   item.  The buffer's size in bytes, starting at position *offset*, must be at
+   least the size required by the format, as reflected by :func:`calcsize`.
 
 
 .. function:: iter_unpack(format, buffer)
@@ -428,7 +428,7 @@ The :mod:`struct` module also defines the following type:
    .. method:: unpack_from(buffer, offset=0)
 
       Identical to the :func:`unpack_from` function, using the compiled format.
-      The buffer's size in bytes, minus *offset*, must be at least
+      The buffer's size in bytes, starting at position *offset*, must be at least
       :attr:`size`.
 
 
diff --git a/Lib/test/test_struct.py b/Lib/test/test_struct.py
index 8fd56c9..454082e 100644
--- a/Lib/test/test_struct.py
+++ b/Lib/test/test_struct.py
@@ -579,14 +579,22 @@ class StructTest(unittest.TestCase):
         self.check_sizeof('0c', 0)
 
     def test_boundary_error_message(self):
-        regex = (
+        regex1 = (
             r'pack_into requires a buffer of at least 6 '
             r'bytes for packing 1 bytes at offset 5 '
             r'\(actual buffer size is 1\)'
         )
-        with self.assertRaisesRegex(struct.error, regex):
+        with self.assertRaisesRegex(struct.error, regex1):
             struct.pack_into('b', bytearray(1), 5, 1)
 
+        regex2 = (
+            r'unpack_from requires a buffer of at least 6 '
+            r'bytes for unpacking 1 bytes at offset 5 '
+            r'\(actual buffer size is 1\)'
+        )
+        with self.assertRaisesRegex(struct.error, regex2):
+            struct.unpack_from('b', bytearray(1), 5)
+
     def test_boundary_error_message_with_negative_offset(self):
         byte_list = bytearray(10)
         with self.assertRaisesRegex(
@@ -599,16 +607,34 @@ class StructTest(unittest.TestCase):
                 'offset -11 out of range for 10-byte buffer'):
             struct.pack_into('<B', byte_list, -11, 123)
 
+        with self.assertRaisesRegex(
+                struct.error,
+                r'not enough data to unpack 4 bytes at offset -2'):
+            struct.unpack_from('<I', byte_list, -2)
+
+        with self.assertRaisesRegex(
+                struct.error,
+                "offset -11 out of range for 10-byte buffer"):
+            struct.unpack_from('<B', byte_list, -11)
+
     def test_boundary_error_message_with_large_offset(self):
         # Test overflows cause by large offset and value size (issue 30245)
-        regex = (
+        regex1 = (
             r'pack_into requires a buffer of at least ' + str(sys.maxsize + 4) +
             r' bytes for packing 4 bytes at offset ' + str(sys.maxsize) +
             r' \(actual buffer size is 10\)'
         )
-        with self.assertRaisesRegex(struct.error, regex):
+        with self.assertRaisesRegex(struct.error, regex1):
             struct.pack_into('<I', bytearray(10), sys.maxsize, 1)
 
+        regex2 = (
+            r'unpack_from requires a buffer of at least ' + str(sys.maxsize + 4) +
+            r' bytes for unpacking 4 bytes at offset ' + str(sys.maxsize) +
+            r' \(actual buffer size is 10\)'
+        )
+        with self.assertRaisesRegex(struct.error, regex2):
+            struct.unpack_from('<I', bytearray(10), sys.maxsize)
+
     def test_issue29802(self):
         # When the second argument of struct.unpack() was of wrong type
         # the Struct object was decrefed twice and the reference to
diff --git a/Misc/NEWS.d/next/Library/2018-03-11-00-20-26.bpo-30249.KSkgLB.rst b/Misc/NEWS.d/next/Library/2018-03-11-00-20-26.bpo-30249.KSkgLB.rst
new file mode 100644
index 0000000..f30dff3
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-03-11-00-20-26.bpo-30249.KSkgLB.rst
@@ -0,0 +1,2 @@
+Improve struct.unpack_from() exception messages for problems with the buffer
+size and offset.
diff --git a/Modules/_struct.c b/Modules/_struct.c
index 66f74d6..0539706 100644
--- a/Modules/_struct.c
+++ b/Modules/_struct.c
@@ -1553,7 +1553,8 @@ Return a tuple containing unpacked values.
 
 Values are unpacked according to the format string Struct.format.
 
-The buffer's size in bytes, minus offset, must be at least Struct.size.
+The buffer's size in bytes, starting at position offset, must be
+at least Struct.size.
 
 See help(struct) for more on format strings.
 [clinic start generated code]*/
@@ -1561,16 +1562,38 @@ See help(struct) for more on format strings.
 static PyObject *
 Struct_unpack_from_impl(PyStructObject *self, Py_buffer *buffer,
                         Py_ssize_t offset)
-/*[clinic end generated code: output=57fac875e0977316 input=97ade52422f8962f]*/
+/*[clinic end generated code: output=57fac875e0977316 input=cafd4851d473c894]*/
 {
     assert(self->s_codes != NULL);
 
-    if (offset < 0)
+    if (offset < 0) {
+        if (offset + self->s_size > 0) {
+            PyErr_Format(StructError,
+                         "not enough data to unpack %zd bytes at offset %zd",
+                         self->s_size,
+                         offset);
+            return NULL;
+        }
+
+        if (offset + buffer->len < 0) {
+            PyErr_Format(StructError,
+                         "offset %zd out of range for %zd-byte buffer",
+                         offset,
+                         buffer->len);
+            return NULL;
+        }
         offset += buffer->len;
-    if (offset < 0 || buffer->len - offset < self->s_size) {
+    }
+
+    if ((buffer->len - offset) < self->s_size) {
         PyErr_Format(StructError,
-            "unpack_from requires a buffer of at least %zd bytes",
-            self->s_size);
+                     "unpack_from requires a buffer of at least %zu bytes for "
+                     "unpacking %zd bytes at offset %zd "
+                     "(actual buffer size is %zd)",
+                     (size_t)self->s_size + (size_t)offset,
+                     self->s_size,
+                     offset,
+                     buffer->len);
         return NULL;
     }
     return s_unpack_internal(self, (char*)buffer->buf + offset);
diff --git a/Modules/clinic/_struct.c.h b/Modules/clinic/_struct.c.h
index 6e43215..2ecadfb 100644
--- a/Modules/clinic/_struct.c.h
+++ b/Modules/clinic/_struct.c.h
@@ -79,7 +79,8 @@ PyDoc_STRVAR(Struct_unpack_from__doc__,
 "\n"
 "Values are unpacked according to the format string Struct.format.\n"
 "\n"
-"The buffer\'s size in bytes, minus offset, must be at least Struct.size.\n"
+"The buffer\'s size in bytes, starting at position offset, must be\n"
+"at least Struct.size.\n"
 "\n"
 "See help(struct) for more on format strings.");
 
@@ -302,4 +303,4 @@ exit:
 
     return return_value;
 }
-/*[clinic end generated code: output=9119f213a951e4cc input=a9049054013a1b77]*/
+/*[clinic end generated code: output=d79b009652ae0b89 input=a9049054013a1b77]*/