Opened 2 years ago

Closed 13 months ago

#5780 defect closed fixed (fixed)

words: Unbound variable nickname in IRCUser for invalid user name

Reported by: j2a Owned by: tom.prince
Priority: normal Milestone:
Component: words Keywords:
Cc: ralphm Branch: branches/unbound-nick-5780
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

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 j2a 2 years ago.
Fix for UnboundLocalError and test for it
5780v2.patch (2.2 KB) - added by stephsolis 15 months ago.
Addressed review suggestions
5780v3.patch (2.6 KB) - added by stephsolis 14 months ago.
Addressed more review suggestions.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc ralphm added

Changed 2 years ago by j2a

Fix for UnboundLocalError and test for it

comment:2 Changed 2 years ago by j2a

  • Summary changed from [patch] words: Unbound variable nickname in IRCUser for invalid user name to words: Unbound variable nickname in IRCUser for invalid user name
  • Type changed from enhancement to defect

comment:3 Changed 2 years ago by exarkun

  • Keywords review added

Pretty sure j2a meant to do this...

comment:4 Changed 2 years ago by tenth

  • Owner set to tenth

comment:5 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from tenth to j2a

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 15 months ago by stephsolis

  • Owner changed from j2a to stephsolis
  • Status changed from new to assigned

Changed 15 months ago by stephsolis

Addressed review suggestions

comment:7 Changed 15 months ago by stephsolis

  • Keywords review added
  • Owner stephsolis deleted
  • Status changed from assigned to new

comment:8 Changed 14 months ago by glyph

  • Author set to glyph
  • Branch set to branches/unbound-nick-5780

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

comment:9 Changed 14 months ago by glyph

  • Keywords review removed
  • Owner set to stephsolis

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 14 months ago by stephsolis

Addressed more review suggestions.

comment:10 Changed 14 months ago by stephsolis

  • Keywords review added
  • Owner stephsolis deleted

comment:11 Changed 14 months ago by tomprince

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

Refs: #5780

comment:12 Changed 13 months ago by tom.prince

  • Keywords review removed
  • Owner set to tom.prince
  • Status changed from new to assigned

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

comment:13 Changed 13 months 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 13 months ago by tomprince

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 13 months ago by thijs

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 13 months ago by tomprince

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Reopens: #5780

enforcenews.py should be smarter about this.

comment:17 Changed 13 months ago by tomprince

  • Resolution set to fixed
  • Status changed from reopened to closed

(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.