Fix remote use of gef.memory.maps

[gordonmessmer]

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.

[github-actions]

🤖 Coverage update for 5c68a97 🟢

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

[gordonmessmer]

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.

[github-actions]

🤖 Coverage update for 2e6166f 🟢

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

[gordonmessmer]

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]

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

[github-actions]

🤖 Coverage update for 8cd572b 🟢

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

[gordonmessmer]

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]

(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.)

[github-actions]

🤖 Coverage update for 4443d0d 🟢

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

[gordonmessmer]

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]

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

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

[github-actions]

🤖 Coverage update for c37d58a 🟢

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

[github-actions]

🤖 Coverage update for fec8600 🟢

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

[hugsy]

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

[gordonmessmer]

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.

[github-actions]

🤖 Coverage update for ee8b8ba 🟢

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

[github-actions]

🤖 Coverage update for 58546bf 🟢

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

[gordonmessmer]

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.

[github-actions]

🤖 Coverage update for bcaddfc 🟢

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

[gordonmessmer]

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]

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

[hugsy]

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]

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]

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

[gordonmessmer]

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]

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]

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]

I am testing with a local gdb server so shouldn’t they match?

…

On Mon, Jul 22, 2024 at 9:12 PM Gordon Messmer ***@***.***> wrote: 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. — Reply to this email directly, view it on GitHub <#1109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZNHQW2L3IG7SMVZCTYX3ZNWUXPAVCNFSM6AAAAABIN74GRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBUGA3DONJWGM> . You are receiving this because you commented.Message ID: ***@***.***>

[gordonmessmer]

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

[Grazfather]

Thanks. I’ll take a look and try to debug when I’m at a computer.

…

On Mon, Jul 22, 2024 at 11:06 PM Gordon Messmer ***@***.***> wrote: Paths will often not match due to the bug we discussed in #1101 (comment) <#1101 (comment)> — Reply to this email directly, view it on GitHub <#1109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZNHS6TTZNPJU7DLWWGNTZNXCDJAVCNFSM6AAAAABIN74GRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBUGE3DENBVGI> . You are receiving this because you commented.Message ID: ***@***.***>

[github-actions]

🤖 Coverage update for 472161b 🟢

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

[gordonmessmer]

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]

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!

[github-actions]

🤖 Coverage update for 071fa4b 🟢

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

[github-actions]

🤖 Coverage update for 9ee31c4 🟢

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

[mahaloz]

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

[gordonmessmer]

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]

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

[github-actions]

🤖 Coverage update for 6e5a96d 🟢

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

[gordonmessmer]

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

[hugsy]

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

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

[gordonmessmer]

Sorry, I meant 6e5a96d

[hugsy]

All merged, great work again @gordonmessmer 🎉