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

Fix remote use of gef.memory.maps #1109

Merged
merged 2 commits into from
Jul 28, 2024
Merged

Conversation

gordonmessmer
Copy link
Contributor

Description

This change passes both the "display" path and the local "real" path for each section or file to the print_got_for function in GotCommand. Without this change, the got command does not work properly in a remote debugging session.

Checklist

  • My code follows the code style of this project.
  • My change includes a change to the documentation, if required.
  • If my change adds new code, adequate tests have been added.
  • I have read and agree to the CONTRIBUTING document.

Copy link

🤖 Coverage update for 5c68a97 🟢

Old New
Commit bdf8219 5c68a97
Score 71.4923% 71.4923% (0)

@gordonmessmer
Copy link
Contributor Author

One way to make this commit a little nicer would be to add an interface that both Sections and the GefSessionManager.file property could implement, each providing both a display path (.path) and a local path (.realpath) attribute/property.

Copy link

github-actions bot commented Jun 1, 2024

🤖 Coverage update for 2e6166f 🟢

Old New
Commit f0b6d1d 2e6166f
Score 71.5991% 71.5991% (0)

@gordonmessmer
Copy link
Contributor Author

I've rebased this commit on the new main branch.

Currently, this patch allows the got command without --all to work, and it allows shared objects whose paths are canonical to work with using the --all option, but non-canonical paths are silently ignored. So, it's partially functional, but not really elegant.

It could be merged as-is. If you have any suggestions on making it better fit the code style of the project, I can work on those. I could also look for a way to print a warning for files that can't be found (which will probably always be non-canonical paths). At a stretch, I could even try to add some simple searches for likely alternate paths when the realpath isn't present.

@hugsy
Copy link
Owner

hugsy commented Jun 1, 2024

No problem. --all is about to be merged anyway.
Also I usually never review draft PR, so when you deem it ready, mark it as such as you can also assign me for review so I get a notification.

Cheers

@gordonmessmer gordonmessmer marked this pull request as ready for review June 1, 2024 19:34
Copy link

github-actions bot commented Jun 1, 2024

🤖 Coverage update for 8cd572b 🟢

Old New
Commit f0b6d1d 8cd572b
Score 71.5991% 71.5991% (0)

@gordonmessmer
Copy link
Contributor Author

I added a patch that performs one permutation to find paths that aren't found at the expected path. With that patch, remote debugging against a Red Hat system provides the full expected output for got --all.

@gordonmessmer
Copy link
Contributor Author

(I did not remove the note about the bug from the got documentation, because shared objects that are not found due to other symlinks could still result in their absence from the output.)

Copy link

github-actions bot commented Jun 1, 2024

🤖 Coverage update for 4443d0d 🟢

Old New
Commit f0b6d1d 4443d0d
Score 71.5991% 71.5991% (0)

@gordonmessmer
Copy link
Contributor Author

Another way to make this implementation less messy would be to always use maps, rather than using maps for --all and gef.session.file/get_filepath otherwise.

I don't have strong feelings about patch 3, but it's an option. I could squash that and the first, or drop it.

@gordonmessmer
Copy link
Contributor Author

"you can also assign me for review so I get a notification"

@hugsy as a non-project member, I don't think I can. 😆

gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
@Grazfather Grazfather requested a review from hugsy June 2, 2024 21:27
Copy link

github-actions bot commented Jun 2, 2024

🤖 Coverage update for c37d58a 🟢

Old New
Commit f0b6d1d c37d58a
Score 71.5991% 71.5991% (0)

Copy link

github-actions bot commented Jun 3, 2024

🤖 Coverage update for fec8600 🟢

Old New
Commit f0b6d1d fec8600
Score 71.6907% 71.6907% (0)

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

It would be nice to add a test case on this function specifically (i.e. search_for_realpath)

gef.py Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Show resolved Hide resolved
@gordonmessmer
Copy link
Contributor Author

So I think the outstanding work is test for search_for_realpath, and make that function private (and assert gef.session.remote).

If I resolved anything by mistake, please let me know.

Copy link

🤖 Coverage update for ee8b8ba 🟢

Old New
Commit f0b6d1d ee8b8ba
Score 71.6798% 71.6798% (0)

Copy link

🤖 Coverage update for 58546bf 🟢

Old New
Commit f0b6d1d 58546bf
Score 71.6798% 71.6798% (0)

@gordonmessmer
Copy link
Contributor Author

I'll have to think about this some more... The test currently checked in succeeds on ubuntu 22.04, but not 20.04.

I could just not assert that a lib in /usr was found, but that opens the possibility of some future test platform not exhibiting this behavior, and the test doing nothing at all.

@gordonmessmer gordonmessmer changed the title Fix remote use of the got command Fix remote use of gef.memory.maps Jun 19, 2024
Copy link

🤖 Coverage update for bcaddfc 🟢

Old New
Commit f0b6d1d bcaddfc
Score 71.6798% 71.6798% (0)

@gordonmessmer
Copy link
Contributor Author

What's important to you in these tests?

Both Debian and Fedora family systems have the same issue that I originally noticed with remote debugging: /lib and /lib64 are symlinks into /usr, and the files retrieved by gdb haven't been resolved by realpath(), so they don't match the paths indicated in /proc/pid/maps. That makes testing easier, because I don't need to introduce tests on a different platform, nor to I need to modify the local tmp root to reproduce the issue.

But Debian also makes ld.so a symlink, and that makes testing on Debian(/Ubuntu) very difficult, because fixing up that path isn't simple and predictable the way that fixing the /usr symlinks is, so remote binaries on Debian systems always have a map that's missing, which looks like a failure in the realpath property of map sections.

On both Ubuntu 20.04 and 22.04, the maps file will contain only the binary executable, ld.so, and virtual objects, typically. But if I add gdb.execute("b main"); gdb.execute("continue") before checking maps, I'll get a copy of maps after runtime linking is complete on Ubuntu 22.04 -- but not on Ubuntu 20.04. That's making testing difficult, because one platform doesn't behave like the other for reasons that aren't obvious to me.

I think the right way to do test the realpath property would be an isolated unit test, not a functional test, but gef isn't designed to be imported for unit testing.

These tests pass, but only because on Ubuntu 20.04 they don't do anything.

@gordonmessmer
Copy link
Contributor Author

Checking in: Let me know if you'd like any more work on this PR.

@hugsy
Copy link
Owner

hugsy commented Jun 29, 2024

Checking in: Let me know if you'd like any more work on this PR.

Hi @gordonmessmer !

I haven't forgotten, but I cannot easily review/test your PR.
I'll try do that on my return next week.

@gordonmessmer
Copy link
Contributor Author

NP. I just like to check in on open PRs every week or two to make sure that I know if someone is waiting on me. :)

@Grazfather
Copy link
Collaborator

Could you come up with step to repro the issue so that I can test it? Let's get this merged :)

@gordonmessmer
Copy link
Contributor Author

gordonmessmer commented Jul 22, 2024

The shortest reproduction case is probably simply:

In one terminal, run gdbserver :9999 /bin/bash

In a second terminal, run gdb with GEF enabled, and then run gef-remote localhost 9999 and got --all.

You'll see the error:

[!] Command 'got' failed to execute properly, reason: unsupported format string passed to GefRemoteSessionManager.__format__

That's because the Section.realpath property is broken, and attempts to use f"/tmp/gef/{gef.session.remote:d}/{self.path}" instead of something like f"{gef.session.remote.root}/{self.path}". But if you fix that and run got --all, you'll still see that the GOT is printed for the main binary but not for any libraries, because no libraries can be found in the remote session root, because the paths in /proc/pid/maps are canonical, but the paths returned by the remote gdbserver are not.

@Grazfather
Copy link
Collaborator

Interestingly, while trying to repro I couldn't get even the format string error. I was sourcing an old work tree, but that means that this is some sort of regression.

On main I can repro and on your branch things mostly look good, but I only get output for the binary e.g. the same output as when I don't use --all

@gordonmessmer
Copy link
Contributor Author

If you don't get any additional libraries, then most likely the paths provided by gdbserver do not match the canonical path for some reason other than a /usr prefix.

Looking at this again, I see that the suffix is also likely to be not canonical due to minor-version sonames, so I'll have to add more code to prune numeric suffixes during the realpath search.

On your system, take a look at /proc/pid/maps for the process you're debugging, and compare the paths that appear in that file to the paths that appear in the gdb session remote root to see if your system has issues other than symlinks to /usr/lib* or the numeric suffixes.

I'll send another patch asap, but if you see something not covered yet, we'd need to expand the search function further.

@Grazfather
Copy link
Collaborator

Grazfather commented Jul 23, 2024 via email

@gordonmessmer
Copy link
Contributor Author

Paths will often not match due to the bug we discussed in #1101 (comment)

@Grazfather
Copy link
Collaborator

Grazfather commented Jul 23, 2024 via email

Copy link

🤖 Coverage update for 472161b 🟢

Old New
Commit f0b6d1d 472161b
Score 71.6649% 71.7025% (0.0376)

@gordonmessmer
Copy link
Contributor Author

I've added another function which should improve the realpath search success rate.

I'd like to believe that this is good enough for now, fixing the vast majority of cases. It's definitely significantly better than the status quo. If more symlinks are identified in the future that the realpath search should test, the function can always be expanded.

@Grazfather
Copy link
Collaborator

Part of my issue was that I was testing on _start, and I had to get to proper main for libc to get loaded and downloaded.

It's looking good now!

gef.py Show resolved Hide resolved
gef.py Show resolved Hide resolved
Copy link

🤖 Coverage update for 071fa4b 🟢

Old New
Commit f0b6d1d 071fa4b
Score 71.6649% 71.6649% (0)

Copy link

🤖 Coverage update for 9ee31c4 🟢

Old New
Commit d8c7723 9ee31c4
Score 71.6649% 71.6649% (0)

@mahaloz
Copy link
Contributor

mahaloz commented Jul 25, 2024

Any way this is related to #1070 that went stale? The library loading part is what makes me think so.

@gordonmessmer
Copy link
Contributor Author

Any way this is related to #1070 that went stale?

Yes and no.

Yes: If the issue that 1070 was intended to fix were fixed, then the tests introduced by this PR would (I think) operate identically on Ubuntu 20.04 and 22.04. And if that happened, then the tests could require that some library realpath was fixed in a remote session. That would be an improvement. And it's possible that this PR will not improve the status quo on Ubuntu 20.04 because the maps file is simply out of date and doesn't refresh.

But otherwise, no. There's minimal relation between the two PRs. Neither PR is a substitute for the other.

@hugsy hugsy self-requested a review July 27, 2024 16:53
@hugsy hugsy added this to the 2024.09 milestone Jul 27, 2024
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Tests are green all across, so LGTM. Just a few minor code quality comments

gef.py Show resolved Hide resolved
tests/api/gef_memory.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
Copy link

🤖 Coverage update for 6e5a96d 🟢

Old New
Commit d8c7723 6e5a96d
Score 71.6649% 71.6649% (0)

@gordonmessmer
Copy link
Contributor Author

Thank you for the review. Feedback should be addressed in the new commit.

@hugsy
Copy link
Owner

hugsy commented Jul 27, 2024

Thank you for the review. Feedback should be addressed in the new commit.

Sweet, I'll merge right after then! Thanks @gordonmessmer

@gordonmessmer
Copy link
Contributor Author

Sorry, I meant 6e5a96d

@hugsy hugsy merged commit 8907935 into hugsy:main Jul 28, 2024
7 checks passed
@hugsy
Copy link
Owner

hugsy commented Jul 28, 2024

All merged, great work again @gordonmessmer 🎉

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

Successfully merging this pull request may close these issues.

4 participants