Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-103464: Add checks for arguments of pdb commands #103465

Merged
merged 5 commits into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 56 additions & 8 deletions Lib/pdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ def do_commands(self, arg):
try:
bnum = int(arg)
except:
self.error("Usage: commands [bnum]\n ...\n end")
self._print_invalid_arg(arg)
return
try:
self.get_bpbynumber(bnum)
Expand Down Expand Up @@ -941,14 +941,22 @@ def do_ignore(self, arg):
condition evaluates to true.
"""
args = arg.split()
try:
count = int(args[1].strip())
except:
if not args:
self.error('Breakpoint number expected')
return
if len(args) == 1:
count = 0
elif len(args) == 2:
try:
count = int(args[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

The current code uses args[1].strip().
Even though I don't have an example for when this would be useful, I suggest not to omit .strip() here to maintain consistency with the rest of the code here.

Copy link
Member Author

Choose a reason for hiding this comment

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

args[1].strip() only strips spaces, which int() takes care of when it tries to convert the string to integer. This is redundant so I removed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, thanks for explaining

except ValueError:
self._print_invalid_arg(arg)
return
else:
self._print_invalid_arg(arg)
return
try:
bp = self.get_bpbynumber(args[0].strip())
except IndexError:
self.error('Breakpoint number expected')
except ValueError as err:
self.error(err)
else:
Expand Down Expand Up @@ -1025,6 +1033,9 @@ def do_where(self, arg):
An arrow indicates the "current frame", which determines the
context of most commands. 'bt' is an alias for this command.
"""
if arg:
self._print_invalid_arg(arg)
return
self.print_stack_trace()
do_w = do_where
do_bt = do_where
Expand Down Expand Up @@ -1112,6 +1123,9 @@ def do_step(self, arg):
(either in a function that is called or in the current
function).
"""
if arg:
self._print_invalid_arg(arg)
return
self.set_step()
return 1
do_s = do_step
Expand All @@ -1122,6 +1136,9 @@ def do_next(self, arg):
Continue execution until the next line in the current function
is reached or it returns.
"""
if arg:
self._print_invalid_arg(arg)
return
self.set_next(self.curframe)
return 1
do_n = do_next
Expand Down Expand Up @@ -1153,6 +1170,9 @@ def do_return(self, arg):

Continue execution until the current function returns.
"""
if arg:
self._print_invalid_arg(arg)
return
self.set_return(self.curframe)
return 1
do_r = do_return
Expand All @@ -1162,6 +1182,9 @@ def do_continue(self, arg):

Continue execution, only stop when a breakpoint is encountered.
"""
if arg:
self._print_invalid_arg(arg)
return
if not self.nosigint:
try:
Pdb._previous_sigint_handler = \
Expand Down Expand Up @@ -1256,6 +1279,9 @@ def do_args(self, arg):

Print the argument list of the current function.
"""
if arg:
self._print_invalid_arg(arg)
return
co = self.curframe.f_code
dict = self.curframe_locals
n = co.co_argcount + co.co_kwonlyargcount
Expand All @@ -1274,6 +1300,9 @@ def do_retval(self, arg):

Print the return value for the last return of a function.
"""
if arg:
self._print_invalid_arg(arg)
return
if '__return__' in self.curframe_locals:
self.message(repr(self.curframe_locals['__return__']))
else:
Expand Down Expand Up @@ -1390,6 +1419,9 @@ def do_longlist(self, arg):

List the whole source code for the current function or frame.
"""
if arg:
self._print_invalid_arg(arg)
return
filename = self.curframe.f_code.co_filename
breaklist = self.get_file_breaks(filename)
try:
Expand Down Expand Up @@ -1570,7 +1602,9 @@ def do_unalias(self, arg):
Delete the specified alias.
"""
args = arg.split()
if len(args) == 0: return
if len(args) == 0:
self._print_invalid_arg(arg)
return
if args[0] in self.aliases:
del self.aliases[args[0]]

Expand Down Expand Up @@ -1723,7 +1757,7 @@ def _getsourcelines(self, obj):
lineno = max(1, lineno)
return lines, lineno

def _help_message_from_doc(self, doc):
def _help_message_from_doc(self, doc, usage_only=False):
lines = [line.strip() for line in doc.rstrip().splitlines()]
if not lines:
return "No help message found."
Expand All @@ -1739,10 +1773,24 @@ def _help_message_from_doc(self, doc):
elif i < usage_end:
prefix = " "
else:
if usage_only:
break
prefix = ""
formatted.append(indent + prefix + line)
return "\n".join(formatted)

def _print_invalid_arg(self, arg):
"""Return the usage string for a function."""

self.error(f"Invalid argument: {arg}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consider using {arg!r} to avoid confusion when arg contains spaces

Copy link
Member Author

Choose a reason for hiding this comment

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

!r simply calls repr() for arg and as arg is str, it should have the identical behavior. Not sure what you are trying to achieve here, could you give an example where !r gives a different result?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I was wrong about repr(str) returns indentical result, don't know what I was thinking. repr() actually returns a worse result:

(Pdb) r '"'
*** Invalid argument: '\'"\''
      Usage: r(eturn)
(Pdb) 

{args} prints exactly what user types and that is more intuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually use repr to add quotes. Didn't realize that there was additional escaping happening. The motivation here is to make arg appear as a single value. This really is just a nit, so...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, taking a look at the rest of the code, I realized people have different ideas about this. The current implementation contains a mix of %r and %s and different efforts to "contain" the argument with ' or (). Some, however, is untouched. It might be beneficial if we could unify them - %r has the escaping side effect and we should use explicit quotes if possible.


# Yes it's a bit hacky. Get the caller name, get the method based on
# that name, and get the docstring from that method.
# This should NOT fail if the caller is a method of this class.
doc = inspect.getdoc(getattr(self, sys._getframe(1).f_code.co_name))
if doc is not None:
self.message(self._help_message_from_doc(doc, usage_only=True))

# Collect all command help into docstring, if not run with -OO

if __doc__ is not None:
Expand Down
36 changes: 33 additions & 3 deletions Lib/test/test_pdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,11 @@ def test_pdb_breakpoint_commands():
(Pdb) commands 10
*** cannot set commands: Breakpoint number 10 out of range
(Pdb) commands a
*** Usage: commands [bnum]
...
end
*** Invalid argument: a
Usage: (Pdb) commands [bpnumber]
(com) ...
(com) end
(Pdb)
(Pdb) commands 4
*** cannot set commands: Breakpoint 4 already deleted
(Pdb) break 6, undefined
Expand Down Expand Up @@ -908,6 +910,34 @@ def test_pdb_skip_modules():
(Pdb) continue
"""

def test_pdb_invalid_arg():
"""This tests pdb commands that have invalid arguments

>>> def test_function():
... import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
... pass

>>> with PdbTestInput([
... 'a = 3',
... 'll 4',
... 'step 1',
... 'continue'
... ]):
... test_function()
> <doctest test.test_pdb.test_pdb_invalid_arg[0]>(3)test_function()
-> pass
(Pdb) a = 3
*** Invalid argument: = 3
Usage: a(rgs)
(Pdb) ll 4
*** Invalid argument: 4
Usage: ll | longlist
(Pdb) step 1
*** Invalid argument: 1
Usage: s(tep)
(Pdb) continue
"""


# Module for testing skipping of module that makes a callback
mod = types.ModuleType('module_to_skip')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Provide helpful usage messages when parsing incorrect :mod:`pdb` commands.