Skip to content

bpo-30566: Fix IndexError in the punycode codec#1986

Closed
vikhegde wants to merge 1 commit into
python:masterfrom
vikhegde:fix-issue-30566
Closed

bpo-30566: Fix IndexError in the punycode codec#1986
vikhegde wants to merge 1 commit into
python:masterfrom
vikhegde:fix-issue-30566

Conversation

@vikhegde

@vikhegde vikhegde commented Jun 7, 2017

Copy link
Copy Markdown

A bug in the punycode codec raises an IndexError when we attempt to decode the string "xn--w&". The fix is straightforward and obvious one-liner. Bug number: bpo-30566

https://bugs.python.org/issue30566

@zware

zware commented Jun 7, 2017

Copy link
Copy Markdown
Member

Could you add a test for this?

@vikhegde

vikhegde commented Jun 7, 2017

Copy link
Copy Markdown
Author

I have added a test case to the punycode Test-suite.

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please also add a NEWS file. See https://devguide.python.org/committing/#what-s-new-and-news-entries for details.

Comment thread Lib/test/test_codecs.py
# Make sure punycode codec raises the right kind of exception
# when given invalid punycode to decode
def test_decode_exceptions(self):
for byt, exception in punycode_decode_exceptions:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rename byt to puny.

Comment thread Lib/test/test_codecs.py
if len(i)!=2:
print(repr(i))

punycode_decode_exceptions = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rename this to invalid_punycode_testcases.

Comment thread Lib/test/test_codecs.py
puny = puny.decode("ascii").encode("ascii")
self.assertEqual(uni, puny.decode("punycode"))

# Make sure punycode codec raises the right kind of exception

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Style nit: We can drop this comment.

Comment thread Lib/test/test_codecs.py
# when given invalid punycode to decode
def test_decode_exceptions(self):
for byt, exception in punycode_decode_exceptions:
with self.assertRaises(exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We couls use self.subTest() to make test failure messages clearer.

@bedevere-bot

Copy link
Copy Markdown

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 phrase I 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.

@serhiy-storchaka serhiy-storchaka changed the title Fix for issue bpo-30566 bpo-30566: Fix IndexError in the punycode codec Mar 26, 2018
@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Mar 26, 2018
@vstinner

Copy link
Copy Markdown
Member

I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches

@zware

zware commented Sep 11, 2019

Copy link
Copy Markdown
Member

@vikhegde Would you like to move this forward by implementing @berkerpeksag's suggestions?

@csabella

Copy link
Copy Markdown
Contributor

This pull request seems to have been abandoned, so I'm going to close it. It can reopened if the original author is interested in working on it again or a new pull request can be created to replace it. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants