Opened 14 months ago

Closed 13 months ago

Last modified 13 months ago

#20030 closed defect (fixed)

meek-http-helper doesn't shutdown cleanly in 6.5a1

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

Description

I've got this process hanging around after I close TB 6.5a2,

/Applications/TorBrowser.app/Contents/MacOS/firefox --invisible -no-remote -profile /Applications/TorBrowser-Data/Tor/PluggableTransports/profile.meek-http-helper

Seems reproducible enough on this day.

Using meek-amazon. meek-azure no longer seems reachable.

Child Tickets

Attachments (1)

0001-Ignore-SIGPIPE-in-meek-client-torbrowser.patch (1.7 KB) - added by dcf 13 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 13 months ago by dcf

Summary: meek-http-helper doesn't shutdown cleanly on OS Xmeek-http-helper doesn't shutdown cleanly in 6.5a2

I can reproduce this on 6.5a2 on linux.

While running:

     ├─firefox─┬─tor───meek-client-tor─┬─firefox─┬─firefox
     │         │                       │         ├─{BgHangManager}
     │         │                       │         ├─{Cache2 I/O}
     │         │                       │         ├─{Cert Verify}
     │         │                       │         ├─{Closing Service}
     │         │                       │         ├─{DNS Resolver #1}
     │         │                       │         ├─{Gecko_IOThread}
     │         │                       │         ├─{Hang Monitor}
     │         │                       │         ├─8*[{JS Helper}]
     │         │                       │         ├─{JS Watchdog}
     │         │                       │         ├─{Link Monitor}
     │         │                       │         ├─{SSL Cert #1}
     │         │                       │         ├─{SSL Cert #2}
     │         │                       │         ├─{Socket Thread}
     │         │                       │         ├─{StreamTrans #1}
     │         │                       │         ├─{Timer}
     │         │                       │         ├─{firefox}
     │         │                       │         └─{mozStorage #1}
     │         │                       ├─meek-client───10*[{meek-client}]
     │         │                       └─5*[{meek-client-tor}]

After quitting:

     ├─firefox─┬─firefox
     │         ├─{BgHangManager}
     │         ├─{Cache2 I/O}
     │         ├─{Cert Verify}
     │         ├─{Closing Service}
     │         ├─{DNS Resolver #1}
     │         ├─{Gecko_IOThread}
     │         ├─{Hang Monitor}
     │         ├─8*[{JS Helper}]
     │         ├─{JS Watchdog}
     │         ├─{Link Monitor}
     │         ├─{Socket Thread}
     │         ├─{Timer}
     │         ├─{firefox}
     │         └─{mozStorage #1}

comment:2 Changed 13 months ago by dcf

Strangely enough, if I add -log meek-client-torbrowser.log after meek-client-torbrowser in Browser/TorBrowser/Data/Tor/torrc-defaults, then it kills the process cleanly. Adding the logging lines just causes a few log messages about killing the two child processes:

2016/09/14 19:31:27 sig terminated
2016/09/14 19:31:27 sending signal terminated to PID 4885
2016/09/14 19:31:27 killing PID 4885
2016/09/14 19:31:27 killing PID 4856

Maybe related to #15125.

comment:3 Changed 13 months ago by mcs

Cc: mcs added

comment:4 Changed 13 months ago by dcf

Summary: meek-http-helper doesn't shutdown cleanly in 6.5a2meek-http-helper doesn't shutdown cleanly in 6.5a1

The first release that has a problem is 6.5a1. I tested:

6.0.3 - ok
6.0.4 - ok
6.0.5 - ok

6.0a5 - ok
6.5a1 - bad
6.5a2 - bad

comment:5 Changed 13 months ago by dcf

I've isolated the problem to the meek-client-torbrowser file. If you take a fresh installation of 6.0a5 (which does not have this bug), and copy Browser/TorBrowser/Tor/PluggableTransports/meek-client-torbrowser from 6.5a1, then the bug occurs.

The meek-client-torbrowser in 6.0a5 and 6.5a1 are built from slightly different sources: 0.22-18371-2 versus 0.22-18371-3. That, however, doesn't seem to be the cause of the problem.

The actual problem seems to be changed Go version, from 1.4.2 to 1.6.2. If I recompile meek-client-torbrowser 0.22-18371-3 with go1.3.3 from Debian stable, then the error does not occur. If I compile it with go1.7.1 from Debian testing, the error does occur.

comment:6 in reply to:  2 Changed 13 months ago by dcf

I tested compiling with various versions of Go and the earliest version that doesn't work is 1.6. 1.5.4 works, 1.6 doesn't.

Strangely enough, if I add -log meek-client-torbrowser.log after meek-client-torbrowser in Browser/TorBrowser/Data/Tor/torrc-defaults, then it kills the process cleanly.

Here's what's going wrong: once it receives a signal, meek-client-torbrowser writes to its log as it shuts down. By default the log lines go to stderr. But meek-client-torbrowser's stderr is the same as its parent process's (tor's) stderr. Once tor terminates, trying to write to the log results in a SIGPIPE that kills meek-client-torbrowser before it can terminate its subprocesses.

Why this only started happening with Go 1.6? See this excerpt from the release notes:

On Unix-like systems, when a write to os.Stdout or os.Stderr (more precisely, an os.File opened for file descriptor 1 or 2) fails due to a broken pipe error, the program will raise a SIGPIPE signal. By default this will cause the program to exit; this may be changed by calling the os/signal Notify function for syscall.SIGPIPE. A write to a broken pipe on a file descriptor other 1 or 2 will simply return syscall.EPIPE (possibly wrapped in os.PathError and/or os.SyscallError) to the caller. The old behavior of raising an uncatchable SIGPIPE signal after 10 consecutive writes to a broken pipe no longer occurs.

That's why logging to a file avoided the error: the output pipe was no longer being broken.

comment:7 Changed 13 months ago by dcf

Keywords: regression added
Status: newneeds_review

attachment:0001-Ignore-SIGPIPE-in-meek-client-torbrowser.patch works around the problem by ignoring SIGPIPE. I found I had to call signal.Notify with a dummy channel; calling signal.Ignore didn't work.

I think we're fine with ignoring logging errors. The log package ignores them anyway. I'm a bit worried that because Go's SIGPIPE rule affects both stdout and stderr, something might go wrong with the PT negotiation on stdout. goptlib doesn't report errors that may arise from its writes to stdout. But I couldn't think of a way it would cause a problem, and even if it does it should be rare.

arlolra, if you want to test this, you can just go build in the meek-client-torbrowser subdirectory, then copy meek-client-torbrowser into your bundle directory.

Last edited 13 months ago by dcf (previous) (diff)

comment:8 Changed 13 months ago by yawning

You can and should do better on Linux, than just ignoring SIGPIPE (which is necessary for OSX).

    cmd = exec.Command(absFirefoxPath, "--invisible", "-no-remote", "-profile", profilePath)
    cmd.SysProcAttr = &syscall.SysProcAttr{ Pdeathsig: syscall.SIGTERM}

comment:9 Changed 13 months ago by arlolra

arlolra, if you want to test this, you can just go build in the meek-client-torbrowser subdirectory, then copy meek-client-torbrowser into your bundle directory.

Yup, seems to work.

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

Resolution: fixed
Status: needs_reviewclosed

Replying to arlolra:

arlolra, if you want to test this, you can just go build in the meek-client-torbrowser subdirectory, then copy meek-client-torbrowser into your bundle directory.

Yup, seems to work.

Thanks, merged in 125b0ca1.

Replying to yawning:

You can and should do better on Linux, than just ignoring SIGPIPE (which is necessary for OSX).

cmd = exec.Command(absFirefoxPath, "--invisible", "-no-remote", "-profile", profilePath)
cmd.SysProcAttr = &syscall.SysProcAttr{ Pdeathsig: syscall.SIGTERM}

Good idea, did this in a3cc514d.

comment:11 Changed 13 months ago by dcf

Ticket #20290 asks to include this fix in Tor Browser.

Note: See TracTickets for help on using tickets.