NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

bpo-37367: octal escapes applied inconsistently throughout the interpreter and lib#14654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

websurfer5 wants to merge2 commits intopython:mainfromwebsurfer5:fix-issue-37367

Closed

bpo-37367: octal escapes applied inconsistently throughout the interpreter and lib#14654

websurfer5 wants to merge2 commits intopython:mainfromwebsurfer5:fix-issue-37367

Conversation

Copy link

Contributor

websurfer5 commentedJul 9, 2019•
edited by bedevere-bot
Loading

Add a range check for octal values in byte strings that raises ValueError if the value is more than 255 (0o377).

Tests for all 1-, 2-, and 3-digit octal values are provided (both valid and error cases).

https://bugs.python.org/issue37367

bpo-37367: raise ValueError for byte strings containing octal

b3459cb

values greater than 255 (0o377)

the-knights-who-say-ni added the CLA signed label

Jul 9, 2019

bedevere-bot added the awaiting review label

Jul 9, 2019

jdemeyer requested changes

Jul 9, 2019

View reviewed changes

Copy link

Contributor

jdemeyer left a comment

Choose a reason for hiding this comment

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

Looks good, apart from a few nitpicks.

Lib/test/test_bytes.py OutdatedShow resolvedHide resolved

Misc/NEWS.d/next/Core and Builtins/2019-07-08-19-08-38.bpo-37367.SVtBhh.rst OutdatedShow resolvedHide resolved

bedevere-bot added awaiting core review and removed awaiting review labels

Jul 9, 2019

brettcannon added the type-bugAn unexpected behavior, bug, or error label

Jul 11, 2019

bpo-37367: changes per reviewer request:

b521f71

1. use "octal escape sequences" instead of "octal values" in comments2. remove underscore from _literal_evalAlso, use "0o1000-0o400" instead of "256" to make it clear to the readerwhere the value came from.

taleinat reviewed

Nov 17, 2019

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2019-07-08-19-08-38.bpo-37367.SVtBhh.rst

		@@ -0,0 +1,2 @@
		Byte strings containing octal escape sequences with values greater than 255
		will now raise ValueError. Patch by Jeffrey Kintscher.

Copy link

Contributor

Choose a reason for hiding this comment

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

It would be helpful to mention the previous behavior here.

taleinat reviewed

Nov 17, 2019

View reviewed changes

Lib/test/test_bytes.py

		importtempfile
		importtextwrap
		importunittest
		importast

Copy link

Contributor

Choose a reason for hiding this comment

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

Please keep the imports sorted alphabetically.

Copy link

Contributor

taleinat commentedNov 17, 2019

@websurfer5, could you merge the master branch into this PR's branch, resolving the merge conflicts?

serhiy-storchaka requested changes

Mar 19, 2022

View reviewed changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please add also a not in the What's New document. Since it is a breaking change it should be in section "Porting to Python 3.11".

Lib/test/test_bytes.py

		c=b.translate(None,delete=b'e')
		self.assertEqual(c,b'hllo')

		deftest_octal_values(self):

Copy link

Member

Choose a reason for hiding this comment

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

It is not correct place for such test. This file is for tests of bytes and bytearray methods, not parsing Python code.

test_string_literals is for parsing string and bytes literals. Add some tests there.

Lib/test/test_codecs.py

		check(br"[\101]",b"[A]")
		check(br"[\1010]",b"[A0]")
		check(br"[\501]",b"[A]")
		self.assertRaises(ValueError,decode,br"[\501]")

Copy link

Member

Choose a reason for hiding this comment

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

Move it to test_errors below.

Objects/bytesobject.c

		}
		if (c>255) {
		PyErr_Format(PyExc_ValueError,
		"octal value must be in range(0, 256)");

Copy link

Member

Choose a reason for hiding this comment

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

The error message should contain the position of the invalid octal escape sequence. It would be nice if look similar to error message for invalid hexadecimal escape sequence. For example:

"invalid octal escape sequence at position %zd"

bedevere-bot removed the awaiting core review label

Mar 19, 2022

Copy link

bedevere-bot commentedMar 19, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.