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

GDB Refactor [21/N]: Move symbol.py to gdblib #1259

Merged
merged 8 commits into from
Oct 11, 2022

Conversation

gsingh93
Copy link
Member

@gsingh93 gsingh93 commented Oct 9, 2022

A couple of changes to symbol.py other than the move:

  • Private methods/variables now start with an underscore, making it easier to understand to understand methods/variables are being accessed from outside the module
  • Added a few type annotations
  • Fixed an issue where we were passing a byte string to pwndbg.symbol.address
  • Removed the handling of integers or strings representing integers from pwndbg.symbol.address. I didn't see any usages of this in the codebase, and allowing so many arbitrary formats will lead to bugs
  • Exceptions from gdb.lookup_symbol are no longer ignored, except for the one case of "No frame selected" errors. This is only occurring in the Go typeinfo tests, and I think the exception probably makes sense, but I want to dig into it a bit more and confirm the right fix is to use gdb.selected_frame() first.
  • Removed the legacy way of looking up symbols with info address

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2022

Codecov Report

Merging #1259 (4143d55) into dev (ec8addd) will decrease coverage by 0.06%.
The diff coverage is 62.10%.

@@            Coverage Diff             @@
##              dev    #1259      +/-   ##
==========================================
- Coverage   54.52%   54.45%   -0.07%     
==========================================
  Files         182      182              
  Lines       20217    20214       -3     
  Branches     1865     1865              
==========================================
- Hits        11023    11008      -15     
- Misses       8761     8783      +22     
+ Partials      433      423      -10     
Impacted Files Coverage Δ
pwndbg/commands/elf.py 26.31% <0.00%> (ø)
pwndbg/commands/probeleak.py 25.80% <0.00%> (ø)
pwndbg/gdblib/elf.py 40.08% <0.00%> (ø)
pwndbg/ghidra.py 16.27% <0.00%> (ø)
pwndbg/heap/ptmalloc.py 41.39% <34.48%> (ø)
pwndbg/commands/windbg.py 84.64% <50.00%> (ø)
pwndbg/gdblib/symbol.py 43.64% <56.00%> (ø)
pwndbg/commands/context.py 62.41% <66.66%> (ø)
pwndbg/arguments.py 59.09% <100.00%> (ø)
pwndbg/chain.py 81.03% <100.00%> (ø)
... and 17 more

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 Author

gsingh93 commented Oct 9, 2022

@CptGibbon @lebr0nli this PR is surfacing any exceptions that were previously caught when doing symbol lookup, and only ignoring a small set of them. On Ubuntu 18.04 and 20.04 (but not 22.04), I see the following exception in test_malloc_chunk_command_heuristic:
image

Do either of you know why that's happening? What's changed in libc since 20.04 that would result in this?

@CptGibbon
Copy link
Contributor

Looks like the test binary needs to be linked against libpthread, I can submit a PR

@disconnect3d disconnect3d merged commit bfbb2b8 into pwndbg:dev Oct 11, 2022
# lookup a symbol. We want to raise these errors so we can handle them
# properly, but there are some we haven't figured out how to fix yet, so
# we ignore those here
skipped_exceptions = []
Copy link
Member

@disconnect3d disconnect3d Oct 11, 2022

Choose a reason for hiding this comment

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

Should be:

skipped_exceptions = (
    # comment here
    "No frame selected",
    ...
)

but this can be changed in a next PR as well

@gsingh93 gsingh93 changed the title Move symbol.py to gdblib GDB Refactor [21/N]: Move symbol.py to gdblib Oct 13, 2022
@gsingh93 gsingh93 deleted the gdblib-symbol branch October 18, 2022 15:41
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