tor sets `TOR_PT_EXIT_ON_STDIN_CLOSE=1` only for server transports, not client transports

At comment:2:ticket:24642, we were surprised to find that server transports get TOR_PT_EXIT_ON_STDIN_CLOSE=1 set, and client transports do not.

https://gitweb.torproject.org/tor.git/tree/src/or/transports.c?id=9e8b762fcecfece64aae70ae640aaa59fd227ca5#n1387

We expected that it would be set for both. Client PTs have code to deal with TOR_PT_EXIT_ON_STDIN_CLOSE, which it turns out has been going unused.

changed milestone to %Tor: 0.4.1.x-final

added component::core tor/tor milestone::Tor: 0.4.1.x-final priority::medium resolution::fixed reviewer::ahf severity::minor status::closed type::defect labels

The commit where the env var went in for server-side was fda61e03 from yawning.

Yawning, do you remember why you put the env var in only on the server side?

I'm also cc'ing ahf here, since he's the closest we've got to the PT interface expert right now.

Trac:
Cc: N/A to yawning, ahf

David, is this bug preventing specific expected behavior from client-side transports right now? Assuming yes, can you give us some specifics? (E.g. "when meek-browser closes on the client side, Tor doesn't notice that it's finished so it doesn't report any problems to the user")

Replying to arma:

David, is this bug preventing specific expected behavior from client-side transports right now? Assuming yes, can you give us some specifics?

No, it's not causing any problems. On non-Windows platforms, tor sends a SIGTERM, which the client transport plugin can catch and terminate cleanly. On Windows, tor has no choice but to call TerminateProcess, which doesn't give the child process a chance to terminate cleanly. Unclean termination isn't a problem for obfs4proxy, because it doesn't manage any external resources. Unclean termination is a problem for meek-client, because meek-client starts its own child processes that would be left orphaned. That's the reason for the now three-year-old terminateprocess-buffer hack, which interposes a process between tor and meek-client that absorbs TerminateProcess and allows meek-client to terminate cleanly. [[comment:21:ticket:15435|I thought]] we would be able to get rid of terminateprocess-buffer eventually, once tor started supporting TOR_PT_EXIT_ON_STDIN_CLOSE for clients. But it turns out that hasn't happened yet.

It just seems like an oversight, as there's nothing in the spec that says it's only for server transports. I know I always anticipated that it would work for clients as well, and was surprised to learn that it wasn't the case.

(E.g. "when meek-browser closes on the client side, Tor doesn't notice that it's finished so it doesn't report any problems to the user")

You have it backwards: TOR_PT_EXIT_ON_STDIN_CLOSE is for tor to tell a child process to terminate, not to detect when a child process has decided to terminate on its own.

Replying to arma:

The commit where the env var went in for server-side was fda61e03 from yawning.

Yawning, do you remember why you put the env var in only on the server side?

It's a bug (https://trac.torproject.org/projects/tor/ticket/24642#comment:6).

Trac:
Cc: yawning, ahf to ahf

For what it's worth, it's probably fine to just assume that the behavior indicated by the env var is present (or gate it behind a command line option or something), since the main reason the env var exists is to indicate that the tor daemon will maintain stdin (it used to close it immediately prior to exec).

Any semi-modern build of tor will do the right thing there.

http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547

Trac:
bug25614.patch

How's this?

Trac:
Status: new to needs_review

Trac:
Keywords: N/A deleted, 040-backport 035-backport added
Milestone: Tor: unspecified to Tor: 0.4.1.x-final

Trac:
Reviewer: N/A to ahf

In the future, please submit Github pull requests if possible for tor.git since this makes it much easier for us to review after our Travis CI and Appveyor CI have run on the pull request :-)

I've opened a pull request with your patch now.

Let us see if CI likes this: https://github.com/torproject/tor/pull/759

Looks like CI was unhappy. Added https://github.com/torproject/tor/pull/759/commits/c9a9de120f4287ccb5cd3998916cbe7b81dba89a - let's see what it says now.

Patch looks good and CI is happy. Let's get it in.

Trac:
Status: needs_review to merge_ready

Is this a backport candidate? Possibly to 0.3.5?

Replying to nickm:

Is this a backport candidate? Possibly to 0.3.5?

IMO no backport is needed, because current transports all have workarounds for this.

great; merged!

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

No backport.

Trac:
Keywords: 040-backport 035-backport deleted, N/A added

closed

moved to tpo/core/tor#25614 (closed)