Opened 3 years ago

Closed 2 years ago

#21757 closed defect (fixed)

Using Pluggable Transports on tor master is broken

Reported by: gk Owned by: ahf
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: ahf Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While testing a patch for #21753 I realized that PTs in general are currently broken with tor master resulting into output like

CMETHOD obfs2 socks5 127.0.0.1:41925
CMETHOD obfs3 socks5 127.0.0.1:41299
CMETHOD obfs4 socks5 127.0.0.1:34025
CMETHOD scramblesuit socks5 127.0.0.1:35225
CMETHODS DONE'. We only support version '1'
Mar 16 13:03:05.000 [warn] Managed proxy at './TorBrowser/Tor/PluggableTransports/obfs4proxy' failed the configuration protocol and will be destroyed.

if started from Tor Browser.

Bisecting reveals that this got introduced with 6e78ede73f190f3aaf91623a131a20cf031aad7e which fixed #21654.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by ahf

Owner: set to ahf
Status: newaccepted

Looking into it.

comment:2 Changed 3 years ago by ahf

Status: acceptedneeds_review

Landed a possible fix on bugs/21757 on Gitlab: https://gitlab.com/ahf/tor/commits/bugs/21757

Passes tests on FreeBSD. Bootstrap and "normal" usage tested via an obfs4 bridge seems to work fine now.

comment:3 Changed 3 years ago by nickm

Question: Does this need to work when the lines do not arrive "all-at-once" ? In other words, what will the code do if we read the first half of a line, but it has no newline, and the second half of the line comes from the next call to read? And will it matter?

I asked a similar question on #2045 -- but I still don't know if it actually matters here.

comment:4 Changed 2 years ago by nickm_mobile

Status: needs_reviewassigned

Merged this to master; leaving this ticket open and assigned to ahf

comment:5 Changed 2 years ago by ahf

Yep, that is an issue. If a program decides to only write partial lines, we will only read up the first chunk of that line with the current code. I do not believe this is a problem with the current PT/firewall-helper programs, but it will certainly cause confusion for the programmer who runs into this issue the first time.

As it is now, we make a single call to read() in get_string_from_pipe() which a lot of the utility functions used for PT's and our firewall helper program uses. Calling get_string_from_pipe() directly instead of having the file descriptors attached to the main loop is where the issue lies IMO. This means they will stop asking for more data on the first EAGAIN return value and never make any additional calls to read more data. This is an issue in log_from_pipe() as well.

Should we close this ticket as the issue is solved and create a new one where we can discuss how to attach these "rogue file descriptors" to the main loop?

comment:6 Changed 2 years ago by gk

FWIW the latest nightly containing tor based on a18b41cc776196c41cf79bdab25592a7ce8bbcc5 has this issue fixed.

comment:7 Changed 2 years ago by ahf

Resolution: fixed
Status: assignedclosed

Great! I'll close this ticket then.

Note: See TracTickets for help on using tickets.