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

Explicitly close the remote session #896

Merged
merged 3 commits into from Oct 11, 2022
Merged

Explicitly close the remote session #896

merged 3 commits into from Oct 11, 2022

Conversation

liona24
Copy link
Contributor

@liona24 liona24 commented Oct 4, 2022

Description/Motivation/Screenshots

This solves an issue with the __del__ magic method being called unreliably, in particular it will often get called just when creating a new remote session using gef-remote which causes an exception in the __del__ hook:

<method 'disconnect' of 'gdb.EventRegistry' objects> returned a result with an exception set

Because of this exception gdb has to be restarted before using the gef-remote command again.

Against which architecture was this tested ?

"Tested" indicates that the PR works and the unit test (i.e. make test) run passes without issue.

  • x86-32
  • x86-64
  • ARM
  • AARCH64
  • MIPS
  • POWERPC
  • SPARC
  • RISC-V

Checklist

  • My PR was done against the dev branch, not main.
  • 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.

@hugsy
Copy link
Owner

hugsy commented Oct 4, 2022

I don't see any issue with merging this PR, but can you provide an example of how to trigger the issue you experienced in a first place?

@liona24
Copy link
Contributor Author

liona24 commented Oct 4, 2022

Consider something like this:

gef> gef-remote 10.10.10.10 1234
...
gef> detach
gef> gef-remote 10.10.10.10 1234
Error: <method 'disconnect' of 'gdb.EventRegistry' objects> returned a result with an exception set

The second gef-remote will always fail until one restarts gdb. (GNU gdb (GDB) Fedora 12.1-1.fc36)

@hugsy
Copy link
Owner

hugsy commented Oct 4, 2022

If that's the only case, it's possible we miss to catch that type of events. I'll investigate further later.

@liona24
Copy link
Contributor Author

liona24 commented Oct 5, 2022

There does not seem to be another event for that. The exit_handler is definitely invoked, the __del__ invocation is just deferred for some reason. Maybe it's a GC thing, I am not sure ..

@hugsy
Copy link
Owner

hugsy commented Oct 5, 2022

I found yesterday there is a new type of event called events.connection_removed but it seems to be there only for gdb11+. So your solution may be a good universal workaround.

@liona24
Copy link
Contributor Author

liona24 commented Oct 6, 2022

Well the exit_handler is definitely invoked anyway so I am not sure if we really need that event. I also tried if forcing gc.collect() would help, but that does not seem to be the case. I cannot think of any other solution than explicitly cleaning up.
Btw, can you reproduce this? Ubuntu 22.04 in docker also seems to have this issue for me.

Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

__del__ isn't guaranteed to be called when you run del. It'll only be called when the garbage collector runs. This is better.

gef.py Outdated
@@ -3507,6 +3507,7 @@ def exit_handler(_: "gdb.ExitedEvent") -> None:
gef.session.qemu_mode = False
if gef.session.remote:
# make sure the tempdir is trashed
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment doesn't make sense actually

gef.py Outdated
@@ -3507,6 +3507,7 @@ def exit_handler(_: "gdb.ExitedEvent") -> None:
gef.session.qemu_mode = False
if gef.session.remote:
# make sure the tempdir is trashed
gef.session.remote.close()
del(gef.session.remote)
Copy link
Collaborator

Choose a reason for hiding this comment

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

del is a keyword not a function, let's fix it.

@hugsy
Copy link
Owner

hugsy commented Oct 8, 2022

@Grazfather The gc runs every time a refcount hits 0 which will be the case here. In any case it's out of the topic of this PR.
@liona24 I tried to find if existing events would help us but couldn't find any. So let's go with your PR 😀 With the small you gave me here can you write a small regression test? Nothing fancy, just to be sure we keep an eye on it

@Grazfather
Copy link
Collaborator

No, the GC runs when it runs. If the refcount hits 0 that just means the GC will take care of it. Using del on the last ref doesn't guarantee it'll be immediately cleaned up.

@liona24
Copy link
Contributor Author

liona24 commented Oct 9, 2022

I added a simple regression test and applied the other suggestions

@hugsy hugsy requested a review from Grazfather October 9, 2022 21:00
@hugsy hugsy merged commit f2050af into hugsy:dev Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants