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

Add ignore command (from #1257) #1276

Merged
merged 4 commits into from
Oct 13, 2022
Merged

Conversation

erivas
Copy link
Contributor

@erivas erivas commented Oct 12, 2022

Tried to follow implementation outline in #1257 , where this was labeled as good-first-issue/help-wanted.

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2022

Codecov Report

Merging #1276 (9bacc93) into dev (fbedf0b) will increase coverage by 0.00%.
The diff coverage is 61.11%.

@@           Coverage Diff           @@
##              dev    #1276   +/-   ##
=======================================
  Coverage   54.87%   54.87%           
=======================================
  Files         182      183    +1     
  Lines       20267    20284   +17     
  Branches     1878     1882    +4     
=======================================
+ Hits        11121    11131   +10     
- Misses       8727     8734    +7     
  Partials      419      419           
Impacted Files Coverage Δ
pwndbg/commands/ignore.py 56.25% <56.25%> (ø)
pwndbg/commands/__init__.py 69.83% <100.00%> (+0.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gsingh93
Copy link
Member

Thanks! I'll test this out myself, but overall the implementation looks good. Would you also be able to add a test for this command? I think we should test different valid and invalid variants of ignore, where there are no breakpoints, where a non-existent breakpoint number is entered, etc.

if bpnum is None:
bpnum = max((bp.number for bp in gdb.breakpoints()), default=None)

bp = next((bp for bp in gdb.breakpoints() if bp.number == bpnum), None)
Copy link
Member

@disconnect3d disconnect3d Oct 13, 2022

Choose a reason for hiding this comment

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

How about:

  1. Changing this code to:
if bpnum is None:
    bp = max(gdb.breakpoints(), key=lambda bp: bp.number, default=None)
else:
    bp = next((bp for bp in gdb.breakpoints() if bp.number == bpnum), None)
  1. Changing the UX to match the original one a bit more:
pwndbg> help ignore
Set ignore-count of breakpoint number N to COUNT.
Usage is `ignore N COUNT'.

pwndbg> ignore 1 123
Will ignore next 123 crossings of breakpoint 1.

pwndbg> ignore 123 123
No breakpoint number 123.

So:

  • we can show an example in the description
  • display error as 'No breakpoint number .' if it is defined or 'No breakpoints set' if it is not
  • for success path, inform user that we did it via "Will ignore next 123 crossings of breakpoint 1."

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw for a multiline description with examples etc. you can use RawDescriptionHelpFormatter as we do in vmmap command: https://github.com/pwndbg/pwndbg/blob/dev/pwndbg/commands/vmmap.py#L50

Also I am wondering if we can grab last bp by just gdb.breakpoints()[-1] but idk if that is always the case so its safer to do it with max(..)

Copy link
Member

Choose a reason for hiding this comment

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

Hm, so we could have:

bps = gdb.breakpoints()

if not bps:
    print(message.error("No breakpoints set."))

if bpnum is None:
    bp = max(bps, key=lambda bp: bp.number)
else:
    bp = next((bp for bp in bps if bp.number == bpnum), None)

    if bp is None:
        print(message.error("No breakpoint number %d." % bpnum))
        return

bp.ignore_count = count
print("Will ignore next %d crossings of breakpoint %d." % (count, bpnum))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Applied the code changes.

I didn't add an example in the description, as an usage: ... line was already generated. It looks like this for me:

pwndbg> help ignore
usage: ignore [-h] [N] COUNT

Set ignore-count of breakpoint number N to COUNT.

While the ignore count is positive, execution will not stop on the breakpoint.

By default, if `N' is ommitted, the last breakpoint (i.e. greatest breakpoint number) will be used.

positional arguments:
  N           The breakpoint number N.
  COUNT       The number to set COUNT.

options:
  -h, --help  show this help message and exit

@erivas
Copy link
Contributor Author

erivas commented Oct 13, 2022

Thanks! I'll test this out myself, but overall the implementation looks good. Would you also be able to add a test for this command? I think we should test different valid and invalid variants of ignore, where there are no breakpoints, where a non-existent breakpoint number is entered, etc.

Thanks, added a few test cases in a file test_command_ignore.py, trying to follow how other commands are tested.

@gsingh93
Copy link
Member

The tests look great! Any reason you're calling gdb.execute("file " + REFERENCE_BINARY) instead of start_binary(REFERENCE_BINARY) in some of the tests?

@disconnect3d disconnect3d merged commit b5da3e8 into pwndbg:dev Oct 13, 2022
@disconnect3d
Copy link
Member

The tests look great! Any reason you're calling gdb.execute("file " + REFERENCE_BINARY) instead of start_binary(REFERENCE_BINARY) in some of the tests?

Seems like an optimization - you don't always have to start the binary but can only load it to gdb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants