Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enforce timeout on initial socks5 proxy connection #125

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

conduition
Copy link
Contributor

This PR fixes a bug which excepted initial SOCKS5 proxy connection attempts from the punctual enforcement of timeouts.

Before this change, invoking Socks5Stream::connect (or Socks5Stream::connect_with_password) could block for much longer than the configured timeout. In practice, this manifested as Client::from_config apparently failing to respect the timeout specified in the Config passed to it. AFAICT this only applied to SOCKS proxy connections.

Example

To demonstrate, here is a simple example program which attempts to connect to an unreachable electrum server with a 10 second timeout.

use electrum_client::{Client, ConfigBuilder, Socks5Config};

fn main() {
    let proxy = Socks5Config::new("127.0.0.1:9050");
    let config = ConfigBuilder::new()
        .socks5(Some(proxy))
        .timeout(Some(10))
        .build();

    let start = std::time::SystemTime::now();
    let result = Client::from_config(
        "tcp://bejqtnc64qttdempkczylydg7l3ordwugbdar7yqbndck53ukx7wnwad.onion:50001",
        config,
    );
    match result {
        Ok(_) => {
            println!("Successfully connected")
        }
        Err(e) => {
            println!(
                "failed to connect after {:.2}s: {e}",
                start.elapsed().unwrap().as_secs_f64()
            );
        }
    };
}

You'd expect the connection attempt to always fail at around 10 seconds, but in fact most attempts take considerably longer.

$ for i in {1..10} ; do cargo run -q ; done
failed to connect after 7.65s: host unreachable
failed to connect after 47.78s: host unreachable
failed to connect after 18.17s: host unreachable
failed to connect after 29.24s: host unreachable
failed to connect after 16.15s: host unreachable
failed to connect after 14.40s: host unreachable
failed to connect after 16.89s: host unreachable
failed to connect after 9.93s: host unreachable
failed to connect after 8.81s: host unreachable
failed to connect after 17.80s: host unreachable

Cause and Fix

This was happening because the private method Socks5Stream::connect_raw only respected the timeout parameter for the initial connection to the proxy address.

Once that TCP socket is established, the SOCKS5 client code must exchange a couple of messages with the proxy itself: One request/response cycle to authenticate, and then another request/response cycle to configure the forward proxy to the ultimate destination address. The latter of these two request/response cycles could block for long periods of time, in the case where the proxy was responsive but the ultimate destination was unresponsive.

Since no timeout was set on the socket at this stage, the Socks5Stream code would wait for an indefinite amount of time for a reply from the proxy, usually only once the proxy itself times out and finally sends a reply.

My suggested fix in this PR is to set the read/write timeouts immediately on the socket connecting to the proxy, so that if the proxy doesn't reply in time, we return an error to the caller promptly.

@notmandatory notmandatory added this to the Release 0.19.0 milestone Dec 7, 2023
@notmandatory
Copy link
Member

@conduition thanks for the fix, and detailed analysis.

@RCasatta do you have any concerns with this fix? otherwise I'll merge it.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a009e55

@notmandatory
Copy link
Member

@conduition can you repush this change with a signed commit? it's a rule on our repo that commits be signed.

@conduition
Copy link
Contributor Author

@notmandatory My pleasure. Re-pushed with GPG signature. ✔️

@RCasatta
Copy link
Member

RCasatta commented Dec 8, 2023

utACK d8554fb

@notmandatory notmandatory merged commit fd81717 into bitcoindevkit:master Dec 8, 2023
1 check passed
@conduition conduition deleted the socks5-timeout branch December 8, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants