PTs could self-shutdown when they detect their stdout is closed

added component::circumvention/pluggable transport needs-spec-change owner::asn parent::10629 priority::medium resolution::wontfix status::closed type::enhancement labels

Trac:
Description: In [#9330 (moved)] we were exploring solutions to signal a PT to do clean shutdown on Windows. In [#10006 (moved)] dcf suggested a workaround using JobObjects, which has the nice property that the children shutdown even when their parent crashes or is killed (SIGKILL or TerminateProcess).

This raises the valid point, why don't we try to achieve this for all platforms? Since all PTs must already communicate via stdout back to Tor (or any parent process, such as a PT chainer), one way of detecting parent death is to check that stdout is still open.

Example: [http://compgroups.net/comp.unix.programmer/how-to-kill-all-child-when-parent-exits/36841]

We'll need to research whether we must write to the stream to detect it's closed, or if we can get away with doing something like poll or select.

to

In [ticket:9330] we were exploring solutions to signal a PT to do clean shutdown on Windows. In [ticket:10006] dcf suggested a workaround using JobObjects, which has the nice property that the children shutdown even when their parent crashes or is killed (SIGKILL or TerminateProcess).

This raises the valid point, why don't we try to achieve this for all platforms? Since all PTs must already communicate via stdout back to Tor (or any parent process, such as a PT chainer), one way of detecting parent death is to check that stdout is still open.

Example: [http://compgroups.net/comp.unix.programmer/how-to-kill-all-child-when-parent-exits/36841]

We'll need to research whether we must write to the stream to detect it's closed, or if we can get away with doing something like poll or select.

Yes, I like this idea.

Like you said we will need to do some research to see how hard detecting closed streams is in practice, but I think it shouldn't be too hard.

Trac:
Parent: N/A to #10629 (moved)

Trac:
Keywords: N/A deleted, needs-spec-change added

I found that you can make this idea work without any changes to tor. All you have to do is introduce a process that interposes between tor and the pluggable transport, and write the pluggable transport so that it interprets a closed stdin as a command to terminate. tor calls TerminateProcess on the buffer process, which dies immediately. The death of the buffer process closes the stdin of the pluggable transport.

Here's a short implementation of the idea.

https://gitweb.torproject.org/pluggable-transports/meek.git/blob/634c6236d9fbc3471d9561e1d534f326308e536f:/terminateprocess-buffer/main.go The terminateprocess-buffer program is what gets called by tor. terminateprocess-buffer just executes the command given by its arguments and creates a pipe to the subcommand's stdin. The way it looks in torrc is:

ClientTransportPlugin meek exec ./Tor/PluggableTransports/terminateprocess-buffer ./Tor/PluggableTransports/meek-client-torbrowser --url=https://meek-reflect.appspot.com/ --front=www.google.com --log meek-client.log

That is, you just stick "terminateprocess-buffer" in front of the command line you were using before.

I tested it in the tbb-3.5.2.1-meek-5 bundles, where the process tree looks like

tor
  terminateprocess-buffer
    meek-client-torbrowser
      firefox
      meek-client

When tor kills terminateprocess-buffer, meek-client-torbrowser notices and cleans up its own children.

(I've just realized that what's described in this comment differs a bit from the ticket description: it's testing for a closed stdin, not stdout.)

Hmm, I'm thinking of wontfixing this in favor of #15435 (moved) (add a pt env var that indicates that stdin will be kept open, which all new tors will set with appropriate modifications to actually keep stdin in the child open).

Rationale is that, terminateprocess-buffer checks stdin instead of stdout, and the newer ticket has a branch that does the right thing. Thoughts?