Opened 2 years ago

Closed 14 months ago

#21765 closed defect (wontfix)

PortForwardingHelper stdout/stderr log forwarding seems fragile

Reported by: ahf Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: technical-debt, 031-deferred-20170425, 032-unreached, 034-triage-20180328, 034-removed-20180328
Cc: Actual Points:
Parent ID: #25409 Points:
Reviewer: Sponsor:

Description

While working on a fix for a regression I caused in #21757 I noticed that we had code in our utility module that could be affected by the same regression as I was trying to fix.

I've tried to list some of my observations while studying this code:

  • We use non-blocking I/O for stdout and stderr in the spawned tor-fw-helper child process, but we only call log_from_pipe() once immediately after spawning the process. From my quick observation the call to log_form_pipe() almost always result in a EAGAIN and we then return without processing any lines at all since we never actually try to read again.
  • We have duplicated code in handle_fw_helper_line() and log_from_pipe() to handle SPAWN_ERROR_MESSAGE error messages during child creation with a comment related to the first observation.

I tried to test this with a simple dummy PortForwardingHelper script which outputs a set of lines on stdout and stderr, sleeps for a while, then continues to write some more content before it exits and only a subset of these lines are actually forwarded to tor's logging subsystem.

The application seems to work correctly and contact my router, so this issue is purely related to the forwarding of stdout and stderr from the helper process.

Child Tickets

Change History (11)

comment:1 Changed 2 years ago by ahf

A work in progress patch can be found at https://gitlab.com/ahf/tor/commit/2ca32216054803f2e966b7b5e21b9479de8b7758 - note that this depends upon the changes in #21757.

comment:2 Changed 2 years ago by yawning

Somewhat off topic.

I sort of wonder if people actually use the PortForwaringHelper option at all. I assume that most people with the (albeit minimal) expertise required to run a relay can set up port forwarding manually, and while the helper code probably still works, I personally don't trust routers enough to ever recommend that anyone actually use it.

comment:3 Changed 2 years ago by nickm

Yeah. I wonder how long this has been broken for. If the answer was "a fairly long while" and nobody noticed, then maybe we should just drop this option from Tor entirely.

comment:4 Changed 2 years ago by nickm

Keywords: 031-deferred-20170425 added
Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

Triage: batch-defer unowned items of priority Medium or lower to 0.3.2.

comment:5 Changed 21 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Move some 0.3.2 items (fewer than I had expected for now) into Unspecified.

comment:6 Changed 16 months ago by teor

Parent ID: #25409

#25409 will fix this issue by removing the option.

comment:7 Changed 16 months ago by teor

Keywords: technical-debt added
Milestone: Tor: unspecifiedTor: 0.3.4.x-final

comment:8 Changed 15 months ago by nickm

Keywords: 034-triage-20180328 added

comment:9 Changed 15 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:10 Changed 14 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:11 Changed 14 months ago by nickm

Resolution: wontfix
Status: newclosed

This code is getting removed; the ticket is now moot.

Note: See TracTickets for help on using tickets.