Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#6225 closed defect (fixed)

"get_string_from_pipe: Assertion len>0 failed; aborting." in log_from_pipe() for tor-fw-helper

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: andrea Actual Points:
Parent ID: #5557 Points:
Reviewer: Sponsor:

Description

The changes of #5557 introduced this assertion failure when reading the tor-fw-helper exit status in tor_check_port_forwarding().

I think the problem was introduced because the new format_helper_exit_status() appends a newline and a NUL to the message. When fgets() in get_string_from_pipe() loops and reads that message, it first stops reading at the newline, and then on the second iteration it reads the NUL which is a zero-length string.

Maybe format_helper_exit_status() should return the length of useful data it wrote, and then only write() that many bytes to stdout in tor_spawn_background()?

(You can reproduce this by using PortForwarding with a wrong PortForwardingHelper path.)

Child Tickets

Change History (7)

comment:1 Changed 8 years ago by nickm

We should also remove that assertion: a subprocess shouldn't be able to crash the main process so easily.

comment:2 Changed 8 years ago by nickm

Cc: andrea added
Status: newneeds_review

Please review and test branch "bug6225" in my public repository. "It works for me!"

comment:3 Changed 8 years ago by andrea

Looks okay to me; I'm not sure I fully understand how this broke anything though. I wrote a newline and a NUL because that's what the old format_helper_exit_status() did.

comment:4 Changed 8 years ago by andrea

Allow me to amend my previous remarks. The old version never wrote the NUL because the write() in tor_spawn_background() only ever wrote HEX_ERRNO_SIZE bytes and the NUL was always past that. *sigh*

Anyway, yeah, your fix looks like it nails that.

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

thanks for the review! merging.

comment:6 Changed 8 years ago by nickm

Keywords: tor-relay added

comment:7 Changed 8 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.