bpo-29649: struct.pack_into check boundary error message didn't respect offset #424

andrewnester · 2017-03-03T12:41:27Z

Fix for https://bugs.python.org/issue29649

mention-bot · 2017-03-03T12:41:30Z

@andrewnester, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Haypo, @mdickinson, @Yhg1s, @serhiy-storchaka and @benjaminp to be potential reviewers.

berkerpeksag · 2017-03-03T13:25:49Z

Lib/test/test_struct.py

@@ -578,6 +578,14 @@ def test__sizeof__(self):
        self.check_sizeof('0s', 1)
        self.check_sizeof('0c', 0)

+    def test_boundary_error_message(self):
+        import ctypes


I'd suggest moving this import module level and use the following pattern:

try: import ctypes except ImportError: ctypes = None

then add

@unittest.skipUnless(ctypes, 'requires ctypes') def test_boundary_error_message(self): ...

berkerpeksag · 2017-03-03T13:27:18Z

Lib/test/test_struct.py

+        byte_list = ctypes.create_string_buffer(1)
+        try:
+            struct.pack_into('b', byte_list, 5, 1)
+        except struct.error as e:


Using assertRaises would be better:

with self.assertRaises(struct.error) as cm: struct.pack_into('b', byte_list, 5, 1) self.assertEqual(str(cm.exception), '...')

Or assertRaisesRegex() if we want to be more lenient to error message.

berkerpeksag · 2017-03-03T13:28:02Z

Misc/NEWS

@@ -9,6 +9,7 @@ What's New in Python 3.7.0 alpha 1?

 Core and Builtins
 -----------------
+- bpo-29649: struct.pack_into check boundary error message didn't respect offset.


This needs to be moved to "Library" section. And please add an empty line after the section header.

serhiy-storchaka · 2017-03-03T13:32:24Z

Lib/test/test_struct.py

@@ -578,6 +578,14 @@ def test__sizeof__(self):
        self.check_sizeof('0s', 1)
        self.check_sizeof('0c', 0)

+    def test_boundary_error_message(self):
+        import ctypes
+        byte_list = ctypes.create_string_buffer(1)


Using ctypes looks unnecessary here. Why not use bytearray?

@serhiy-storchaka absolutely agree, thanks!

serhiy-storchaka · 2017-03-03T13:35:14Z

Lib/test/test_struct.py

+        byte_list = ctypes.create_string_buffer(1)
+        try:
+            struct.pack_into('b', byte_list, 5, 1)
+        except struct.error as e:


Or assertRaisesRegex() if we want to be more lenient to error message.

serhiy-storchaka · 2017-03-04T13:36:21Z

Lib/test/test_struct.py

+    def test_boundary_error_message(self):
+        byte_list = bytearray(1)
+        with self.assertRaisesRegex(struct.error,
+                                    'pack_into requires a buffer of at least 6 bytes for packing 1 bytes at offset 5'):


Too long line.

serhiy-storchaka · 2017-03-04T13:38:32Z

Misc/NEWS

@@ -261,6 +261,7 @@ Library

 - bpo-29623: Allow use of path-like object as a single argument in
  ConfigParser.read().  Patch by David Ellis.
+- bpo-29649: struct.pack_into check boundary error message didn't respect offset.


The entry should be added at the start of corresponding section. Separate it with empty lines from other text.

Add () after struct.pack_into. Add "Patch by ".

serhiy-storchaka · 2017-03-04T13:49:53Z

Modules/_struct.c

@@ -1937,8 +1937,8 @@ s_pack_into(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames
    /* Check boundaries */
    if (offset < 0 || (buffer.len - offset) < soself->s_size) {
        PyErr_Format(StructError,
-                     "pack_into requires a buffer of at least %zd bytes",
-                     soself->s_size);
+                     "pack_into requires a buffer of at least %zd bytes for packing %zd bytes at offset %zd)",


Too long lines.

serhiy-storchaka

Please also address my comment on the tracker.

serhiy-storchaka · 2017-03-05T15:14:35Z

Misc/NEWS

@@ -259,6 +259,9 @@ Extension Modules
 Library
 -------

+- bpo-29649: struct.pack_into() check boundary error message
+  didn't respect offset. Patch by Andrew Nester.


Use two spaces after sentence ending period.

andrewnester · 2017-03-05T16:32:49Z

Thanks @serhiy-storchaka ! I've just added changes you requested + new error messages based on your comment from tracker. Thanks a lot!

serhiy-storchaka · 2017-03-05T17:12:54Z

Modules/_struct.c

@@ -1930,15 +1930,40 @@ s_pack_into(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames
        return NULL;
    }

+    /* Check that negative offset is low enough to fit data */
+    if (offset < 0 && offset + soself->s_size > 0) {


I think three following if's can be merged into one if (offset < 0). This will speed up common case of non-negative offset.

serhiy-storchaka · 2017-03-05T17:14:25Z

Modules/_struct.c

+    /* Check that negative offset is low enough to fit data */
+    if (offset < 0 && offset + soself->s_size > 0) {
+        PyErr_Format(StructError,
+                     "pack_into requires negative offset not higher than %zd "


This just repeats the same number two times (but with different signs). It would be more helpful to output an actual value of offset.

serhiy-storchaka · 2017-03-05T17:17:59Z

Modules/_struct.c

-                     soself->s_size);
+                     "pack_into requires a buffer of at least %zd bytes for "
+                     "packing %zd bytes at offset %zd",
+                     soself->s_size + (offset > 0 ? offset : 0),


I think it would be more helpful to output an actual size of the buffer rather of soself->s_size + offset. The user can add two reported numbers.

@serhiy-storchaka I would like to leave required size indicated explicitly but will add actual buffer size to this error message. Thanks!

andrewnester · 2017-03-05T18:22:07Z

thanks @serhiy-storchaka for review, I just made couple of additional changes you requested.

andrewnester · 2017-03-09T09:25:59Z

@serhiy-storchaka @berkerpeksag any updates on this?

serhiy-storchaka · 2017-03-09T10:00:04Z

"not higher/lower than some negative value" doesn't look pretty clear. I afraid it can cause confusion. I prefer to output the offset and the size of the buffer or data. Something like

pack_into() can't pack into buffer of size 10 at offset -11
pack_into() can't pack the data of size 11 at offset -10
pack_into() can't pack the data of size 11 into buffer of size 20 at offset 10

Maybe other developers can suggest better wording.

vadmium

@andrewnester’s current proposals are

1: pack_into requires a buffer of at least R bytes for packing P bytes at offset O (actual buffer size is B)
2: pack_into requires negative offset not higher than H (actual offset is O)
3: pack_into requires negative offset not lower than L (actual offset is O)

I agree the negative offset ones are awkward. My suggestions:

2: No space to pack 11 bytes at offset −10
3: Offset −11 out of range for 10-byte buffer

vadmium · 2017-03-11T03:50:08Z

Lib/test/test_struct.py

+                struct.error,
+                'pack_into requires a buffer of at least 6 '
+                'bytes for packing 1 bytes at offset 5 '
+                '\(actual buffer size is 1\)'):


I think it would be good to use a raw string here: r'\(. . .\)'

andrewnester · 2017-03-11T08:42:20Z

thanks @vadmium @serhiy-storchaka
I just changed error messages to ones proposed by @vadmium
Thanks!

vadmium · 2017-03-12T23:46:09Z

Lib/test/test_struct.py

+                struct.error,
+                r'pack_into requires a buffer of at least 6 '
+                'bytes for packing 1 bytes at offset 5 '
+                '\(actual buffer size is 1\)'):


Correct me if I am wrong (I haven’t caught up with Python after the Git Hub changeover), but I think you need to make the final part of the string a raw string:

>>> (r'pack_into requires a buffer of at least 6 ' ... 'bytes for packing 1 bytes at offset 5 ' ... '\(actual buffer size is 1\)') <stdin>:3: DeprecationWarning: invalid escape sequence \(

@vadmium Thanks! you're right, I updated my PR.

…ct offset

berkerpeksag · 2017-04-04T01:05:38Z

Lib/test/test_struct.py

@@ -578,6 +578,26 @@ def test__sizeof__(self):
        self.check_sizeof('0s', 1)
        self.check_sizeof('0c', 0)

+    def test_boundary_error_message(self):
+        byte_list = bytearray(1)


Do we need byte_list? Can't we just pass bytearray(1) to pack_into()?

berkerpeksag · 2017-04-04T01:07:48Z

Lib/test/test_struct.py

+        byte_list = bytearray(1)
+        with self.assertRaisesRegex(
+                struct.error,
+                'pack_into requires a buffer of at least 6 '


This is just a style nit, but I find this style is a bit ugly. I prefer something like this:

regex = ( r'multi' r'line' r'regex' ) with self.assertRaisesRegex(struct.error, regex): ...

berkerpeksag · 2017-04-04T01:11:07Z

Modules/_struct.c

+         /* Check that negative offset is low enough to fit data */
+        if (offset + soself->s_size > 0) {
+            PyErr_Format(StructError,
+                         "No space to pack %zd bytes at offset %zd",


I admit that we are pretty inconsistent with the styles of our exception messages, but I think these two should start with lowercase letters.

berkerpeksag · 2017-04-04T01:14:05Z

Misc/NEWS

@@ -322,7 +325,7 @@ Library
  Now using them emits a deprecation warning.

 - bpo-27863: Fixed multiple crashes in ElementTree caused by race conditions
-  and wrong types. 
+  and wrong types.


This looks like an unrelated change to me.

berkerpeksag · 2017-04-04T01:20:04Z

Misc/NEWS

@@ -303,6 +303,9 @@ Extension Modules
 Library
 -------

+- bpo-29649: struct.pack_into() check boundary error message


Perhaps something like "Exception message of struct.pack_into() now respects [...]" might be better. It's 4am here so perhaps @vadmium can come up with a better wording :)

It’s 1 p.m. here! Maybe something like “Improve ‘struct.pack_into’ exception messages for problems with the buffer size and offset”.

andrewnester · 2017-04-04T09:16:49Z

@berkerpeksag @vadmium thanks for the review, seems reasonable. All changes done. Could you please review it again? Thanks!

berkerpeksag

LGTM, thanks! I will let Serhiy do the merging since he knows this part of CPython better than myself.

Bumps [celery](https://github.com/celery/celery) from 5.0.4 to 5.0.5. - [Release notes](https://github.com/celery/celery/releases) - [Changelog](https://github.com/celery/celery/blob/master/Changelog.rst) - [Commits](celery/celery@v5.0.4...v5.0.5) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

the-knights-who-say-ni added the CLA signed label Mar 3, 2017

berkerpeksag requested changes Mar 3, 2017

View reviewed changes

serhiy-storchaka reviewed Mar 3, 2017

View reviewed changes

serhiy-storchaka reviewed Mar 4, 2017

View reviewed changes

serhiy-storchaka requested changes Mar 5, 2017

View reviewed changes

serhiy-storchaka reviewed Mar 5, 2017

View reviewed changes

serhiy-storchaka requested a review from vadmium March 9, 2017 09:59

vadmium reviewed Mar 11, 2017

View reviewed changes

vadmium reviewed Mar 12, 2017

View reviewed changes

serhiy-storchaka approved these changes Mar 13, 2017

View reviewed changes

andrewnester added 4 commits April 2, 2017 18:39

bpo-29649: struct.pack_into check boundary error message didn't respe…

21872ce

…ct offset

Code styles fixes

e3fb912

Added new error messages for improper negative offset.

11d21c0

Changed error messages

35b354f

berkerpeksag reviewed Apr 4, 2017

View reviewed changes

Minor code style fixes

8870d3c

berkerpeksag approved these changes Apr 4, 2017

View reviewed changes

serhiy-storchaka merged commit f78b119 into python:master Apr 4, 2017

Uh oh!

bpo-29649: struct.pack_into check boundary error message didn't respect offset #424

bpo-29649: struct.pack_into check boundary error message didn't respect offset #424

Uh oh!

Conversation

andrewnester commented Mar 3, 2017

Uh oh!

mention-bot commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnester commented Mar 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnester commented Mar 5, 2017

Uh oh!

andrewnester commented Mar 9, 2017

Uh oh!

serhiy-storchaka commented Mar 9, 2017

Uh oh!

vadmium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnester commented Mar 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnester commented Apr 4, 2017

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!