Opened 5 weeks ago

Last modified 9 days ago

#24642 needs_review defect

cannot use TOR_PT_EXIT_ON_STDIN_CLOSE with meek-client-torbrowser

Reported by: mcs Owned by: dcf
Priority: Medium Milestone:
Component: Obfuscation/meek Version:
Severity: Normal Keywords:
Cc: brade Actual Points:
Parent ID: #24689 Points:
Reviewer: Sponsor:

Description

I am using meek built from the 0.28 tag on macOS.

If I add TOR_PT_EXIT_ON_STDIN_CLOSE=1 to the environment and start meek-client-torbrowser, the meek-client process that is started exits immediately. It generates this message on its way out:

synthesizing SIGTERM because of stdin close

I think this happens because meek-client-torbrowser does not attach stdin to anything when starting meek-client.

I am not sure how best to fix this. One approach would be for meek-client-torbrowser to remove TOR_PT_EXIT_ON_STDIN_CLOSE=1 from the environmnt before starting meek-client (since meek-client-torbrowser already handles TOR_PT_EXIT_ON_STDIN_CLOSE=1 on its own).

Child Tickets

Change History (8)

comment:1 Changed 5 weeks ago by mcs

Parent ID: #24689

comment:2 Changed 12 days ago by dcf

You're right. This is a bug in meek-client-torbrowser.

Better than unsetting TOR_PT_EXIT_ON_STDIN_CLOSE, I think, is to actually give the meek-client subprocess a stdin, something like this:

@@ -268,6 +268,7 @@ func runMeekClient(helperAddr string, meekClientCommandLine []string) (cmd *exec
        args := meekClientCommandLine[1:]
        args = append(args, []string{"--helper", helperAddr}...)
        cmd = exec.Command(meekClientPath, args...)
+       cmd.Stdin = os.Stdin
        cmd.Stdout = os.Stdout
        cmd.Stderr = os.Stderr
        log.Printf("running meek-client command %q", cmd.Args)

Another (possibly better) option is to call cmd.StdinPipe() and just never close the pipe (that way the child process's stdin is separate from the parent's, so you don't have a race between them trying to terminate when the stdin is closed).

I was pretty surprised by this, because I was under the impression that tor always sets TOR_PT_EXIT_ON_STDIN_CLOSE=1 nowadays, and therefore this code should never have worked in any released bundle. But it turns out it only sets the variable for server transports, not client transports, which is why this bug wasn't detected earlier:
https://gitweb.torproject.org/tor.git/tree/src/or/transports.c?id=9e8b762fcecfece64aae70ae640aaa59fd227ca5#n1387

What do you need from me, a new tag with this bug fixed?

comment:3 in reply to:  2 ; Changed 11 days ago by mcs

Replying to dcf:

You're right. This is a bug in meek-client-torbrowser.

Better than unsetting TOR_PT_EXIT_ON_STDIN_CLOSE, I think, is to actually give the meek-client subprocess a stdin, something like this:
...
Another (possibly better) option is to call cmd.StdinPipe() and just never close the pipe (that way the child process's stdin is separate from the parent's, so you don't have a race between them trying to terminate when the stdin is closed).

The above makes sense to me. cmd.StdinPipe() seems like a good solution. Kathy and I can test such a patch if you'd like us to do so.

...
What do you need from me, a new tag with this bug fixed?

Yes.

comment:4 in reply to:  2 ; Changed 11 days ago by yawning

Replying to dcf:

I was pretty surprised by this, because I was under the impression that tor always sets TOR_PT_EXIT_ON_STDIN_CLOSE=1 nowadays, and therefore this code should never have worked in any released bundle. But it turns out it only sets the variable for server transports, not client transports, which is why this bug wasn't detected earlier:
https://gitweb.torproject.org/tor.git/tree/src/or/transports.c?id=9e8b762fcecfece64aae70ae640aaa59fd227ca5#n1387

I don't remember why it only does that on servers. Should it set the env var for clients as well?

comment:5 in reply to:  4 ; Changed 11 days ago by dcf

Replying to yawning:

Replying to dcf:

I was pretty surprised by this, because I was under the impression that tor always sets TOR_PT_EXIT_ON_STDIN_CLOSE=1 nowadays, and therefore this code should never have worked in any released bundle. But it turns out it only sets the variable for server transports, not client transports, which is why this bug wasn't detected earlier:

I don't remember why it only does that on servers. Should it set the env var for clients as well?

I had assumed that it was set for clients too. I can't think of a reason why it wouldn't be (except for this bug, of course).

The purpose of the terminateprocess-buffer program, which we still use on Windows, was to simulate support for TOR_PT_EXIT_ON_STDIN_CLOSE in tor clients that don't have it. In my reasoning three years ago I thought that TOR_PT_EXIT_ON_STDIN_CLOSE was coming for clients and that terminateprocess-buffer would not be necessary someday.

But that brings up another question: terminateprocess-buffer sets TOR_PT_EXIT_ON_STDIN_CLOSE=1 unconditionally (and also provides its child process with a stdin). So it seems that this bug should have arisen on Windows, with meek-client exiting immediately because it doesn't have a stdin.

The documentation for exec.Cmd says:

If Stdin is nil, the process reads from the null device (os.DevNull).

Where os.DevNull is NUL on Windows. So all I can think of is that NUL doesn't provide an immediate EOF like /dev/null does. Or maybe I am missing something else. I need to run some more tests.

comment:6 in reply to:  5 Changed 11 days ago by yawning

Replying to dcf:

Replying to yawning:

Replying to dcf:

I was pretty surprised by this, because I was under the impression that tor always sets TOR_PT_EXIT_ON_STDIN_CLOSE=1 nowadays, and therefore this code should never have worked in any released bundle. But it turns out it only sets the variable for server transports, not client transports, which is why this bug wasn't detected earlier:

I don't remember why it only does that on servers. Should it set the env var for clients as well?

I had assumed that it was set for clients too. I can't think of a reason why it wouldn't be (except for this bug, of course).

I certainly intended for it to be everywhere when I added the construct, and obfs4proxy checks the env var regardless of if it is a client or server. I looked back at my commit, and the code's unchanged from when I added the patch, so I guess it's a bug that's always been there.

comment:7 in reply to:  3 ; Changed 10 days ago by dcf

Status: newneeds_review

Replying to mcs:

Replying to dcf:

What do you need from me, a new tag with this bug fixed?

Yes.

The bug24642 branch uses the StdinPipe idea.

I'm not totally happy with it, because ideally according to pt-spec, we should keep track of the stdin handle, and close it before sending anything like SIGTERM to the subprocess. But that would require more rewriting and is more than you need right now.

comment:8 in reply to:  7 Changed 9 days ago by mcs

Replying to dcf:

The bug24642 branch uses the StdinPipe idea.

I tested it and it seems to work fine. Thanks!!!

I'm not totally happy with it, because ideally according to pt-spec, we should keep track of the stdin handle, and close it before sending anything like SIGTERM to the subprocess. But that would require more rewriting and is more than you need right now.

Yes, we are happy with the simpler fix for now (but what you said does makes sense).

Note: See TracTickets for help on using tickets.