Fix windows `Socket::connect_timeout` overflow by eval-exec · Pull Request #112464 · rust-lang/rust

eval-exec · 2023-06-09T13:47:12Z

This PR want to close#112405

add unit test

rustbot · 2023-06-09T13:47:21Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

eval-exec · 2023-06-09T14:19:43Z

@rustbot review

ChrisDenton · 2023-06-10T15:46:13Z

This looks good to me.

add unit test

This is a bit more tricky but I think the easiest way would be to copy/paste connect_error and then change it to use connect_timeout(&next_test_ip4(), Duration::MAX) instead of connect.

ChrisDenton · 2023-06-12T09:34:55Z

Have you confirmed this test fails without the changes in net.rs?

@eval-exec for an alternative idea of what a test could do:
bind some free port on localhost using TcpListener::bind, then
connect to that new port with Duration::MAX timeout.
Here's a draft in Rust:
use std::net::TcpListener;let listener = TcpListener::bind("127.0.0.1:0").unwrap();// :0 picks some free portlet port = listener.local_addr().unwrap().port();// obtain the port it picked[..][..]format!("127.0.0.1:{port}").as_str()[..]// connect to that port
I have a real example of something very close at https://github.com/hartwork/rust-for-it/blob/4e51982c4465c3190d92a59747c5d2738baf9e07/src/network.rs#L160-L177 .
What do you think?

Thank you, I'm testing it on a win10 virtual machine.

@eval-exec I checked the latest version: 0.0.0.0:1 will only work if you start a TCP listener on port 1. And port 1 only works if it's unoccupied and you have privileges because ports up to 1024 are reserved. In a test context you'll probably want a dynamic free port as in my example above.

I'm confused, I try to connect to an unreachable address 8.8.8.8:9999, but it didn't get timeout.

@eval-exec updated twice more (in case you only saw the version sent via notification e-mail)

It should be format!("0.0.0.0:{port}" or format!("127.0.0.1:{port}" for connect_timeout?

@eval-exec I would argue for 127.0.0.1 for clarity. Both work in practice.

I think I should bind and accept in another thread:
#[test]fnconnect_timeout_ok_bind(){let socket_addr = next_test_ip4(); thread::spawn({let listener = t!(TcpListener::bind(&socket_addr));move || {letmut stream = t!(listener.accept()).0;letmut buf = [0];t!(stream.read(&mut buf));}});assert!(TcpStream::connect_timeout(&socket_addr,Duration::MAX).is_ok());}

I think I should bind and accept in another thread:
@eval-exec that's not necessary, see https://github.com/hartwork/rust-for-it/blob/4e51982c4465c3190d92a59747c5d2738baf9e07/src/network.rs#L160-L177 .

thomcc · 2023-06-17T06:32:28Z

Chris seems like he's reviewing this.

r? @ChrisDenton

ChrisDenton · 2023-06-17T22:17:02Z

The failure condition in your issue was "Instead, this happened: the call never succeeds, not even for google.com:80 on Windows."

That is not infinite blocking.

hartwork · 2023-06-17T23:48:39Z

The failure condition in your issue was "Instead, this happened: the call never succeeds, not even for google.com:80 on Windows."
That is not infinite blocking.

@ChrisDenton maybe I should have put it more clearly: by "never succeeds" I meant that it should have returned success quickly for google.com:80 but instead it blocked forever and hence never finishes to return with due success. Given your spot-on early analysis at #112405 (comment) I do not understand how we have different understanding now. I'd be happy to learn what I a missing now. Are there two bugs here instead of a single one?

hartwork · 2023-06-17T23:53:57Z

@ChrisDenton PS: The CI at https://github.com/hartwork/rust-for-it/actions/runs/5204380736/jobs/9388510541 was cancelled by GitHub after near-exact 6 hours runtime due to that call while google.com:80 was available.

eval-exec · 2023-06-18T00:01:46Z

@ChrisDenton PS: The CI at https://github.com/hartwork/rust-for-it/actions/runs/5204380736/jobs/9388510541 was cancelled by GitHub after near-exact 6 hours runtime due to that call while google.com:80 was available.

What's the related code of this canceled CI action?

hartwork · 2023-06-18T00:13:24Z

@ChrisDenton PS: The CI at https://github.com/hartwork/rust-for-it/actions/runs/5204380736/jobs/9388510541 was cancelled by GitHub after near-exact 6 hours runtime due to that call while google.com:80 was available.
What's the related code of this canceled CI action?

@eval-exec that would effectively be https://github.com/hartwork/rust-for-it/tree/4c816e51eb8134cef32571400f5583e3f75cec54 or:

git clone https://github.com/hartwork/rust-for-it
cd rust-for-it
git checkout 4c816e51eb8134cef32571400f5583e3f75cec54

eval-exec · 2023-06-18T01:08:45Z

connect to a reachable address:

use std::net::{TcpStream,ToSocketAddrs};use std::time::Duration;fnmain(){let address_result = "1.1.1.1:80".to_socket_addrs();let address = address_result.unwrap().next().unwrap();TcpStream::connect_timeout(&address,Duration::MAX).expect("connect_timeout");println!("main fn returned");}

On Linux platform, rustc 1.70.0, this program returned imediately, and no error.
On Windows platform, rustc 1.70.0, this program panic at an error imediately: thread 'main' panicked at 'connect_timeout: Error { kind: TimedOut, message: "connection timed out" }', src\main.rs:7:57
On Windows platform, with this PR's fix, and I run this program in a unit test, this program returned imediately, and no error

connect to an unreachable address:

use std::net::{TcpStream,ToSocketAddrs};use std::time::Duration;fnmain(){let address_result = "1.1.1.1:9999".to_socket_addrs();let address = address_result.unwrap().next().unwrap();TcpStream::connect_timeout(&address,Duration::MAX).expect("connect_timeout");println!("main fn returned");}

On linux platform, rustc 1.70.0, this program running 2minutes and 8second, and finaly got thread 'main' panicked at 'connect_timeout: Os { code: 110, kind: TimedOut, message: "Connection timed out" }', src/main.rs:7:57
On Windows platform, rustc 1.70.0, this program panic at an error imediately: thread 'main' panicked at 'connect_timeout: Error { kind: TimedOut, message: "connection timed out" }', src\main.rs:7:57
On Windows platform, with this PR's fix, and I run this program in a unit test, this program paniced after 21.04 seconds:

thread 'net::tcp::tests::connect_timeout_example' panicked at 'connect_timeout: Os { code: 10060, kind: TimedOut, message: "A connection attempt failed because the connected p
arty did not properly respond after a period of time, or established connection failed because connected host has failed to respond." }', library\std\src\net\tcp\tests.rs:15:88

eval-exec · 2023-06-18T01:16:22Z

So, I think I can add a unit test: connect to an unreachable address like 1.1.1.1:9999. assertconnect_timeout(1.1.1.1:9999, Duration::MAX) will get a TimedOut error, and assert the action should cost at least 20 seconds.

#[test]fnconnect_timeout_to_unreachable_address(){let now = Instant::now();let result =
TcpStream::connect_timeout(&format!("1.1.1.1:9999").parse().unwrap(),Duration::MAX);assert!(matches!(result,ErrorKind::TimedOut));assert!(now.elapsed() > Duration::from_secs(20));}

What do you think? @hartwork @ChrisDenton

ChrisDenton · 2023-06-18T07:19:05Z

@hartwork the reason your CI ran indefinitely is because you have an infinite loop. if running only the code in the issue, it errors immediately.

Signed-off-by: Eval EXEC <execvy@gmail.com>

eval-exec · 2023-06-18T08:02:23Z

 }

+#[test]
+fn connect_timeout_to_unreachable_address() {


I tested, this unit test will cost at 2 minutes and 8 seconds on Linux platform, and cost 20 seconds on Windows.

I think we only need one test here. We only need to focus on testing the problematic code path in one way. 2 minutes and 8 secs is a long time for a single unit test so I'd prefer if we just used connect_timeout_error test and removed the other two.

Ok, I think 2 minutes and 8 seconds is a long time too.

hartwork · 2023-06-18T17:33:24Z

@hartwork the reason your CI ran indefinitely is because you have an infinite loop. if running only the code in the issue, it errors immediately.

@ChrisDenton thanks for clearing this up, you're 100% right, I was able to confirm your results in GitHub Actions now. Sorry 🙏

Co-authored-by: Chris Denton <christophersdenton@gmail.com>

ChrisDenton

This looks great to me, thanks! Sorry testing end up being so involved.

ChrisDenton · 2023-06-20T10:57:30Z

@bors r+ rollup

bors · 2023-06-20T10:57:32Z

📌 Commit a0c757a has been approved by ChrisDenton

It is now in the queue for this repository.

eval-exec · 2023-06-20T10:59:12Z

Thank you.

…

On Tue, Jun 20, 2023 at 6:57 PM Chris Denton ***@***.***> wrote: ***@***.**** approved this pull request. This looks great to me, thanks! Sorry testing end up being so involved. — Reply to this email directly, view it on GitHub <#112464 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALCAINR7SJLSDNMQPA2SILLXMF6ZRANCNFSM6AAAAAAZAVVJSE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#112464 (Fix windows `Socket::connect_timeout` overflow) - rust-lang#112720 (Rustdoc: search: color item type and reduce size to avoid clashing) - rust-lang#112762 (Sort the errors from arguments checking so that suggestions are handled properly) - rust-lang#112786 (change binders from tuple structs to named fields) - rust-lang#112794 (Fix linker failures when #[global_allocator] is used in a dependency) - rust-lang#112819 (Disable feature(unboxed_closures, fn_traits) in weird-exprs) r? `@ghost` `@rustbot` modify labels: rollup

RalfJung · 2026-05-12T17:36:55Z

+fn connect_timeout_error() {
+ let socket_addr = next_test_ip4();
+ let result = TcpStream::connect_timeout(&socket_addr, Duration::MAX);
+ assert!(!matches!(result, Err(e) if e.kind() == ErrorKind::TimedOut));


This is a strange test. It says to only timeout after billions of years, yet apparently it expects the OS to timeout immediately. What is going on here? And why is there no comment explaining this?^^
Context: #156385
View changes since the review

Oh, I missed a ! :)

Hello, I reviewed your PR. Is there some tasks we can do next for further steps?

If the comment there makes sense then I think that's all, thanks :)

rustbot assigned thomcc Jun 9, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 9, 2023

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch 2 times, most recently from bb125c5 to 80b5ea8Compare June 9, 2023 14:18

ChrisDenton reviewed Jun 10, 2023
View reviewed changes

Comment threadlibrary/std/src/sys/windows/net.rs Outdated

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch 2 times, most recently from 26720ce to 845c89cCompare June 10, 2023 14:04

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 1e6a863 to e23346bCompare June 11, 2023 01:06

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from e23346b to 2953b71Compare June 11, 2023 01:16

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 2953b71 to fc4cff2Compare June 11, 2023 01:46

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from fc4cff2 to 7bac07dCompare June 11, 2023 02:26

eval-exec requested a review from ChrisDenton June 11, 2023 03:16

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 7bac07d to 11df849Compare June 11, 2023 03:40

ChrisDenton reviewed Jun 12, 2023
View reviewed changes

rustbot assigned ChrisDenton and unassigned thomcc Jun 17, 2023

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 11df849 to 7871e56Compare June 17, 2023 08:16

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from a4cce33 to ca83467Compare June 17, 2023 16:20

This comment has been minimized.
Sign in to view

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from 13c995c to bf52e9bCompare June 18, 2023 07:42

This comment has been minimized.
Sign in to view

Add unit test to connect to an unreachable address
f65b5d0
Signed-off-by: Eval EXEC <execvy@gmail.com>

eval-exec force-pushed the exec/fix-connect_timeout-overflow branch from bf52e9b to f65b5d0Compare June 18, 2023 08:00

eval-exec commented Jun 18, 2023
View reviewed changes

ChrisDenton reviewed Jun 20, 2023
View reviewed changes

Comment threadlibrary/std/src/net/tcp/tests.rs Outdated

eval-execand others added 2 commits June 20, 2023 18:43

Ignore connect_timeout unit test on SGX platform
30e1c1a
Co-authored-by: Chris Denton <christophersdenton@gmail.com>

Remove useless unit tests
a0c757a

eval-exec requested a review from ChrisDenton June 20, 2023 10:48

ChrisDenton approved these changes Jun 20, 2023
View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2023

GuillaumeGomez mentioned this pull request Jun 20, 2023
Rollup of 6 pull requests #112839
Merged

bors merged commit 816b659 into rust-lang:masterJun 20, 2023

rustbot added this to the 1.72.0 milestone Jun 20, 2023

RalfJung reviewed May 12, 2026
View reviewed changes

Uh oh!

Uh oh!

Conversation

eval-exec commented Jun 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 9, 2023

Uh oh!

This comment has been minimized.

eval-exec commented Jun 9, 2023

Uh oh!

Uh oh!

ChrisDenton commented Jun 10, 2023

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartworkJun 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eval-execJun 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomcc commented Jun 17, 2023

Uh oh!

This comment has been minimized.

This comment has been minimized.

ChrisDenton commented Jun 17, 2023

Uh oh!

hartwork commented Jun 17, 2023

Uh oh!

hartwork commented Jun 17, 2023

Uh oh!

eval-exec commented Jun 18, 2023

Uh oh!

hartwork commented Jun 18, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eval-exec commented Jun 18, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

connect to a reachable address:

connect to an unreachable address:

Uh oh!

eval-exec commented Jun 18, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisDenton commented Jun 18, 2023

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

ChrisDentonJun 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartwork commented Jun 18, 2023

Uh oh!

Uh oh!

ChrisDenton left a comment

eval-exec commented Jun 9, 2023•
edited
Loading

hartworkJun 17, 2023•
edited
Loading

eval-execJun 17, 2023•
edited
Loading

hartwork commented Jun 18, 2023•
edited
Loading

eval-exec commented Jun 18, 2023•
edited
Loading

eval-exec commented Jun 18, 2023•
edited
Loading

ChrisDentonJun 20, 2023•
edited
Loading

RalfJungMay 12, 2026•
edited
Loading

eval-execMay 14, 2026•
edited
Loading