Misc run_in_new_terminal improvements.

[rickyz]

Summary of changes:

• Prefer tmux/screen over TERM_PROGRAM and x-terminal-emulator.
• Add an option to automatically kill processes started by run_in_new_terminal when the main process exits.

[rickyz]

This was added in 60e5a80. I wasn't able to repro on my Mac OS X machine with tmux 2.8, but should be straightforward to add back if it is still an issue.

[rickyz]

This could kill the wrong process on PID reuse, but unfortunately these are the APIs we have to work with. Hopefully PID reuse is relatively unlikely to happen during the relatively short lifetime a pwntools script.

[rickyz]

It is a little unfortunate that this doesn't work nicely for all terminal types. One way to do it would be to wrap the command in a shell script which dumps its pid to a temp file before execing the command. I didn't want to think too hard about shell escaping, which is why I've settled for making it just work in tmux here. Happy to try out a more general approach if you think it'd be useful though.

[Arusekk]

I guess that it should work quite ok. For shell escaping, there is an excellent sh_string module in pwntools, so you don't have to think so hard :)

[zachriggle]

I don't mind the ordering change (since it's useful both on macOS and when ssh'ed into a remote machine on Linux), but I disagree with the other changes, since they appear to break multiple things. Can we split the other stuff out into a separate PR to discuss?

• Why do we need tmux to tell us the PID? Both os.fork() and subprocess provide better mechanisms for getting the PID. (Edit: I see that tmux forks and kills the process we spawn directly.)
• Why are we changing the default behavior to kill the spawned process? The default should be False or the existing in-library invocations should also be updated to supply False as the argument
  • This does not seem like it will work as expected for the primary use-case. Launching gdb in a new terminal, then debugging a process will only kill gdb and will leave the debuggee hung.

I'm not sure this code was tested, since it breaks immediately if I try to check out this branch.

/Users/riggle/.ipython/profile_default/startup/10-import.py in <module>()
      1 try:
----> 2     from pwn import *
      3 except ImportError as e:
      4     print "Could not load pwntools:", e
      5 import os, re, sys, time, random, urllib, urllib2, datetime, itertools, subprocess, multiprocessing

/Users/riggle/pwntools/pwn/__init__.py in <module>()
     11 args = pwnlib.args.args
     12
---> 13 if not platform.architecture()[0].startswith('64'):
     14     """Determines if the current Python interpreter is supported by Pwntools.
     15

NameError: name 'platform' is not defined

If you fix up the import, it throws an exception if the spawned process dies before Python exits.

>>> run_in_new_terminal('sh')
47813
>>> exit
Traceback (most recent call last):
  File "pwnlib/util/misc.py", line 271, in <lambda>
    atexit.register(lambda: os.kill(pid, signal.SIGTERM))
OSError: [Errno 3] No such process

And on my Mac 10.13.6 with iTerm2 3.2.7 and tmux 2.7 it does not actually kill the spawned process if it is left alive and the interpreter closes

[rickyz]

I don't mind the ordering change (since it's useful both on macOS and when ssh'ed into a remote machine on Linux), but I disagree with the other changes, since they appear to break multiple things. Can we split the other stuff out into a separate PR to discuss?

Will split out next chance I get to poke at this. Just responding quickly to some of the comments for now.

• Why are we changing the default behavior to kill the spawned process? The default should be False or the existing in-library invocations should also be updated to supply False as the argument

Agreed that defaulting to False makes more sense. For calls which start a gdb window (which currently, is all calls), I like the idea of defaulting to True, or at least making the default behavior configurable.

• This does not seem like it will work as expected for the primary use-case. Launching gdb in a new terminal, then debugging a process will only kill gdb and will leave the debuggee hung.

In my admittedly brief testing, gdb detaches cleanly from the debuggee on SIGTERM (edit: Though perhaps things change due to the use of gdbserver). My expectation is that this change shouldn't introduce any new debuggee orphaning behavior compared to what happens today. The most common case is probably the debuggee terminating once its stdin is closed. (The second most common case is probably buggy challenges which infinite loop when stdin is closed - I can see the argument that it's nice to have a gdb window to kill those with.) I'll do some more testing of this later.

I'm not sure this code was tested, since it breaks immediately if I try to check out this branch.

Guilty as charged - removing that import was the single last-moment change that I didn't test, because I saw that misc.py did not have any uses of platform. Little did I know that pwn/init.py relied on the side effect of platform being imported. I did test run_in_new_terminal with tmux though.

And on my Mac 10.13.6 with iTerm2 3.2.7 and tmux 2.7 it does not actually kill the spawned process if it is left alive and the interpreter closes

I only tested run_in_new_terminal in tmux 2.8, but I guess this would be a good argument for using the terminal-agnostic approach I mentioned.

[zachriggle]

Though perhaps things change due to the use of gdbserver

The lifetime of the gdbserver is handled separately -- it doesn't go through run_in_new_terminal. "Handled" in this case means "not handled" 😝 but it is entirely separate. We use --once to ensure that it dies when the gdb client disconnects for any reason, and it should terminate after that.

I only tested run_in_new_terminal in tmux 2.8, but I guess this would be a good argument for using the terminal-agnostic approach I mentioned.

We may want to use pwnlib.pids.children to kill all child processes of the one we spawned.

I'd also prefer to use the process tube to launch anything that we need to communicate with -- we shouldn't be using subprocess here directly.

In any case, I'm down to merge the order-of-preference changes in this PR and add the kill-child-process stuff in another PR. I think it makes more sense to offer it as an option to tubes.process(..., kill_children_on_exit=...) and handle it there, and we can leverage it to kill tmux children etc.