#24642 closed defect (fixed)

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 (11)

comment:1 Changed 12 months ago by mcs

Parent ID: #24689

comment:2 Changed 11 months 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 months 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 months 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 months 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 months 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 11 months 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 11 months 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).

comment:9 Changed 11 months ago by mcs

David — if you are willing to live with this fix for now, can you please merge it and then also create a tag for us (#24614)? I would really like to get the Moat feature to the point of code review and testing by someone other than Kathy and myself.

comment:10 in reply to:  9 Changed 10 months ago by dcf

Replying to mcs:

David — if you are willing to live with this fix for now, can you please merge it and then also create a tag for us (#24614)? I would really like to get the Moat feature to the point of code review and testing by someone other than Kathy and myself.

I tagged you 0.29.

comment:11 Changed 10 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

We are done here as it works. dcf: I guess you want to open a new ticket for your preferred fix?

Note: See TracTickets for help on using tickets.