rlcompleter raises Exception on bad input

[donlorenzo]

BPO	2250
Nosy	@nascheme, @birkenfeld
Files	rlcompleter.patch: catches Name- and AttributeError and instead returns an empty list rlcompleter_Exception.patch: catches all Exceptions and returns an empty list instead rlcompleter.rst.diff: note that exceptions will be caught.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/nascheme'
closed_at = <Date 2008-05-11.21:04:17.220>
created_at = <Date 2008-03-07.10:49:56.552>
labels = ['easy', 'type-bug', 'library']
title = 'rlcompleter raises Exception on bad input'
updated_at = <Date 2008-05-11.21:04:17.121>
user = 'https://bugs.python.org/donlorenzo'

bugs.python.org fields:

activity = <Date 2008-05-11.21:04:17.121>
actor = 'georg.brandl'
assignee = 'nascheme'
closed = True
closed_date = <Date 2008-05-11.21:04:17.220>
closer = 'georg.brandl'
components = ['Library (Lib)']
creation = <Date 2008-03-07.10:49:56.552>
creator = 'donlorenzo'
dependencies = []
files = ['9627', '9940', '10055']
hgrepos = []
issue_num = 2250
keywords = ['patch', 'easy']
message_count = 9.0
messages = ['63349', '64206', '64414', '64453', '64554', '64936', '65615', '65616', '66669']
nosy_count = 4.0
nosy_names = ['nascheme', 'georg.brandl', 'jafo', 'donlorenzo']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue2250'
versions = ['Python 2.5']

[donlorenzo]

in line 130 rlcompleter calls eval on the first part (before the last
dot) of the input text.
if that part is not valid it will raise an exception which is likely to
crash a calling application.

example 1:
==========

import rlcompleter
rlcompleter.Completer().complete("foo.", 0) -> raises NameError

example 2:
==========

import rlcompleter
foo = 5
rlcompleter.Completer().complete("foo.bar.", 0) -> raises AttributeError

the patch puts the eval call in a try-except block and catches Name- and
AttributeErrors and returns an empty list (the behavior when no matches
are found).

[jafo]

Is a straightforward patch, but I'd like NAS to comment on the change in
behavior. Probably would also need a documentation change, are you up
for doing that Lorenz?

[donlorenzo]

I have no idea what or who NAS is but comments are always welcome :)

concerning the documentation:
I´ve never done it before but there is a first time for everything,
right? I´m currently on vacation so I won´t look into it for at least
another 2 weeks.

[nascheme]

On Thu, Mar 20, 2008 at 08:49:32PM +0000, Sean Reifschneider wrote:

Is a straightforward patch, but I'd like NAS to comment on the change in
behavior. Probably would also need a documentation change, are you up
for doing that Lorenz?

I doubt that people are relying on complete() to raise NameError or
Attribute error. I think it would be okay just to trap them and not
change the docs. A note in NEWS should be good enough.

Neil

[donlorenzo]

I was thinking that the code in question could maybe also raise other
exceptions. too bad I´m on vacation and can´t try this out myself.
I believe the regular expression also matches something like

import rlcompleter
rlcompleter.Completer().complete("1foo.2bar3.smth", 0)

which I guess would result in a SyntaxError.
would be nice if someone could verify that.

If I´m right I see two possibilities. either change the regular
expression to match only valid python identifieres or also catch
SyntaxErrors.

could there be any other exception? ("In the face of ambiguity, refuse
the temptation to guess.")

[donlorenzo]

I confirmed that the rlcompleter can raise a SyntaxError on bad input.
Upon further investigation I found that a ReferenceError could also be
raised. I didn't check on other Exceptions.

I attached a new version of the patch where I catch all Errors derived
from Exceptions (not SystemError and KeyboardInterrupt).

while this seems rather drastic I really feel that this step should in
no case raise an Exception. I can't possibly think of a situation where
someone would want that.

Concerning the docs:
"...it will try to evaluate anything without obvious side-effects..."
I would consider raising an exception as a major side-effect so this
patch rather brings the code into accordance with the docs than it
contradicts them.

[donlorenzo]

I attached a patch for the docs.
It now states that the rlcompleter.Completer will catch and silence all
exceptions raised by evaluating the expression passed to complete()

[donlorenzo]

In the last patch I used the wrong ticks. I hope it's fine like this.

[birkenfeld]

Committed code and doc patches as r63094. Thanks!