bpo-29649: struct.pack_into check boundary error message didn't respect offset by andrewnester · Pull Request #424 · python/cpython

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

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

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

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

Using ctypes looks unnecessary here. Why not use bytearray?

@serhiy-storchaka absolutely agree, thanks!

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

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

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

Too long line.

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

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

Too long lines.

serhiy-storchaka

Please also address my comment on the tracker.

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

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

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

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

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

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

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

        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

+        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

+         /* 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


 - 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

 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

Comment thread Misc/NEWS Outdated

Copy link
Copy Markdown

Member

serhiy-storchaka Mar 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use two spaces after sentence ending period.

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

Comment thread Lib/test/test_struct.py Outdated

Copy link
Copy Markdown

Member

vadmium Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone