Opened 12 months ago

Closed 2 weeks ago

Last modified 9 days ago

#25614 closed defect (fixed)

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

Reported by: dcf Owned by:
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords:
Cc: ahf Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

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.

Child Tickets

Attachments (1)

bug25614.patch (1.9 KB) - added by dcf 4 weeks ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 12 months ago by arma

Cc: yawning ahf added

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.

comment:2 Changed 12 months ago by arma

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")

comment:3 in reply to:  2 Changed 12 months ago by dcf

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

comment:4 in reply to:  1 Changed 12 months ago by yawning

Cc: yawning removed

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

comment:5 Changed 12 months ago by yawning

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.

Changed 4 weeks ago by dcf

Attachment: bug25614.patch added

comment:7 Changed 4 weeks ago by dcf

Status: newneeds_review

How's this?

comment:8 Changed 4 weeks ago by nickm

Keywords: 040-backport 035-backport added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final

comment:9 Changed 3 weeks ago by asn

Reviewer: ahf

comment:10 Changed 2 weeks ago by 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

comment:11 Changed 2 weeks ago by ahf

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

comment:12 Changed 2 weeks ago by ahf

Status: needs_reviewmerge_ready

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

comment:13 Changed 2 weeks ago by nickm

Is this a backport candidate? Possibly to 0.3.5?

comment:14 in reply to:  13 Changed 2 weeks ago by dcf

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.

comment:15 Changed 2 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

great; merged!

comment:16 Changed 9 days ago by teor

Keywords: 040-backport 035-backport removed

No backport.

Note: See TracTickets for help on using tickets.