Skip to content

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

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

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

Conversation

@vikhegde
Copy link
Copy Markdown

@vikhegde vikhegde commented Jun 7, 2017

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
Copy link
Copy Markdown
Member

zware commented Jun 7, 2017

Could you add a test for this?

@vikhegde
Copy link
Copy Markdown
Author

vikhegde commented Jun 7, 2017

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

Copy link
Copy Markdown
Member

@berkerpeksag berkerpeksag left a comment

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
Copy link
Copy Markdown
Member

zware commented Sep 11, 2019

@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