Using Pluggable Transports on tor master is broken

While testing a patch for legacy/trac#21753 (moved) 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 6e78ede7 which fixed legacy/trac#21654 (moved).

changed milestone to %Tor: 0.3.1.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.1.x-final in Legacy / Trac owner::ahf in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac labels

Looking into it.

Trac:
Status: new to accepted
Owner: N/A to ahf

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.

Trac:
Status: accepted to needs_review

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 legacy/trac#2045 (moved) -- but I still don't know if it actually matters here.

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

Trac:
Status: needs_review to assigned

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?

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

Great! I'll close this ticket then.

Trac:
Status: assigned to closed
Resolution: N/A to fixed

closed

mentioned in issue legacy/trac#21765 (moved)

moved from legacy/trac#21757 (moved)

added Bug label and removed 1 deleted label

removed 1 deleted label