Opened 5 years ago

Closed 4 years ago

#5780 defect closed fixed (fixed)

words: Unbound variable nickname in IRCUser for invalid user name

Reported by: Yury Yurevich Owned by: Tom Prince
Priority: normal Milestone:
Component: words Keywords:
Cc: ralphm Branch: branches/unbound-nick-5780
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

If user send NICK command with non-utf8 non-ascii symbols, IRCUser raises UnboundLocalError:

exceptions.UnboundLocalError: local variable 'nickname' referenced before assignment

patch and test is attached.

Attachments (3)

unbound_local_error.patch (1.7 KB) - added by Yury Yurevich 5 years ago.
Fix for UnboundLocalError and test for it
5780v2.patch (2.2 KB) - added by Stephen Solis 5 years ago.
Addressed review suggestions
5780v3.patch (2.6 KB) - added by Stephen Solis 5 years ago.
Addressed more review suggestions.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: ralphm added

Changed 5 years ago by Yury Yurevich

Attachment: unbound_local_error.patch added

Fix for UnboundLocalError and test for it

comment:2 Changed 5 years ago by Yury Yurevich

Summary: [patch] words: Unbound variable nickname in IRCUser for invalid user namewords: Unbound variable nickname in IRCUser for invalid user name
Type: enhancementdefect

comment:3 Changed 5 years ago by Jean-Paul Calderone

Keywords: review added

Pretty sure j2a meant to do this...

comment:4 Changed 5 years ago by David Sturgis

Owner: set to David Sturgis

comment:5 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from David Sturgis to Yury Yurevich

Thanks.

  1. The docstrings for test_utf8Messages and test_wrongUtf8Nick do half of a good job. They should explain the desired behavior as well as explaining the case they are exercising, though.
  2. Please separate methods from each other by two blank lines and wrap lines at less than 80 columns.
  3. nickname = params[0] isn't likely to raise a UnicodeDecodeError, right? So the line being added inside the try/except could probably go before the try statement begins. It's nice to minimize the amount of code inside try blocks.

Thanks again! It's really great to see you working on this area of Twisted.

comment:6 Changed 5 years ago by Stephen Solis

Owner: changed from Yury Yurevich to Stephen Solis
Status: newassigned

Changed 5 years ago by Stephen Solis

Attachment: 5780v2.patch added

Addressed review suggestions

comment:7 Changed 5 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted
Status: assignednew

comment:8 Changed 5 years ago by Glyph

Author: glyph
Branch: branches/unbound-nick-5780

(In [38481]) Branching to 'unbound-nick-5780'

comment:9 Changed 5 years ago by Glyph

Keywords: review removed
Owner: set to Stephen Solis

Thanks for moving this patch forward, stephsolis. Sorry for the delay on this review; we have quite a backlog right now.

There are now some build results, but due to some recent maintenance, many of the builders are unavailable.

A few minor points:

  1. Please make life a bit easier for the person who eventually ports this code to py3k and use `b''` or `u''` strings?, so that it will be unambiguous whether you're attempting to represent bytes or text with your strings.
  2. Manually typing in UTF-8 escapes to byte strings is unclear. Instead, create a text (unicode) string, then encode it to UTF-8. Practically speaking this just means "use \u instead of \x escapes", but this means it's a lot easier for someone reading the test to tell for example, if your test code points have any surrogate pairs in them.
  3. In point 1 of his previous review, when he said that the docstrings do "half of a good job", exarkun was referring to our preferred convention for test docstrings. Sadly I don't think this is documented officially anywhere, so I'll re-explain it again here. Test docstrings should make a positive statement about the attributes of the desired behavior of the code under test. So saying that something "does not raise an exception" is not helpful; most code doesn't raise an exception and it doesn't do a whole bunch of other things too :). Instead, docstrings should say things like "when a UTF-8 message is sent with sendMessage, it will be written to the transport." Also, if you're going to talk about exceptions, you should talk about specific types; "Exception" is a bit too vague. (It would be silly to talk about NameError though.)
  4. This stuff all talks about UTF-8, but actually the encoding of a particular IRCUser is a settable attribute, so it should talk about the encoding of the user's connection.
  5. This one's just a quibble really; it's "invalid" UTF-8, not "wrong UTF-8", so name / document the test accordingly.
  6. This change, like all changes, needs a NEWS file. I leave it to your discretion whether this should be a bugfix or misc (whether it should be described in the changelog or not, respectively).

If you were a committer, I'd say all these changes were small enough that you could just commit it yourself when you're done. Unfortunately since you're not, I have to ask you to submit another patch to finish this. Feel free to ping me personally to remind me to review it so we can get it merged promptly when you have responded to this review. Thanks again!

Changed 5 years ago by Stephen Solis

Attachment: 5780v3.patch added

Addressed more review suggestions.

comment:10 Changed 5 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted

comment:11 Changed 5 years ago by Tom Prince

(In [38808]) Apply 5780v3.patch from stephsolis.

Refs: #5780

comment:12 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Tom Prince
Status: newassigned

This looks good. I'll merge after the buildbot runs.

comment:13 Changed 4 years ago by Tom Prince

Actually, the newsfile says that it allows NICK commands with non-UTF8 data. But, it doesn't *allow* them, it just properly reports an error, for them.

comment:14 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: assignedclosed

(In [38864]) Merge unbound-nick-5780: Fix handling of invalid UTF-8 in IRC NICK commands.

Author: j2a, stephsolis Reviewers: exarkun, glyph, tom.prince Fixes: #5780

Previously, when sent a NICK command that had invalid UTF-8, it would fail with an UnboundLocalError. It now properly reports an error to the user, instead.

comment:15 Changed 4 years ago by Thijs Triemstra

Sorry for not catching this earlier but the topfile is in the wrong place and this change will only be reported in the core section of the news file, but it should be in words. It's easy to make this mistake, wonder if the commit hook/tools can be smarter about this.

comment:16 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: closedreopened

(In [38871]) Revert r38864: Topfile in the wrong place.

Reopens: #5780

enforcenews.py should be smarter about this.

comment:17 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: reopenedclosed

(In [38872]) Merge unbound-nick-5780: Fix handling of invalid UTF-8 in IRC NICK commands.

Author: j2a, stephsolis Reviewers: exarkun, glyph, tom.prince Fixes: #5780

Previously, when sent a NICK command that had invalid UTF-8, it would fail with an UnboundLocalError. It now properly reports an error to the user, instead.

Note: See TracTickets for help on using tickets.