Fix the issue where loopback-bound sockets attempt to connect to a public IP by magnified103 · Pull Request #3531 · shadow/shadow

magnified103 · 2025-03-12T12:33:48Z

In the current sys_connect handler, sockets bound to localhost are not handled correctly.

shadow/src/main/host/descriptor/socket/inet/legacy_tcp.rs

Lines 760 to 765 in 3e478a9

    
           // a connected tcp socket must be bound 
        
           let is_bound = unsafe { c::legacysocket_isBound(socket_ref.as_legacy_socket()) } == 1; 
        
           if !is_bound { 
        
               log::trace!("Implicitly binding listener socket"); 
        
               // implicit bind: bind to an ephemeral port (use default interface unless the remote

Shadow does not check cases where it connects to an external address using a socket bound to 127.0.0.1, which later causes crashes when the worker fails to resolve the localhost address to a host.

shadow/src/main/core/worker.rs

Lines 356 to 358 in 3e478a9

    
           // check if network reliability forces us to 'drop' the packet 
        
           let reliability: f64 = Worker::with(|w| w.shared.reliability(src_ip, dst_ip).unwrap()) 
        
               .unwrap()

The same issue also occurs with UDP (and possibly in tcp.rs).

This PR brings a fix to this issue, although I think we'd want to mimic a routing table and perform these handling there.

magnified103 · 2025-03-12T17:00:50Z

The current fix was based on two-month-old commits and passed all tests at that time. I'm still investigating why the tests are failing now.

stevenengler · 2025-03-12T17:08:26Z

The current fix was based on two-month-old commits and passed all tests at that time. I'm still investigating why the tests are failing now.

In the past month, we've migrated Shadow to the rust 2024 edition, the rustfmt 2024 edition, and are building with rust 1.85 in the CI now. There have also been some CI fixes due to a breaking change in rustup that may affect older commits that don't have the CI fixes.

You might need to rebase to get the fixes, and then run rustfmt and clippy to make sure they still pass.

stevenengler · 2025-03-12T17:13:15Z

Also thanks for the PR! One additional thing we'll need is some tests in src/test/socket/ to ensure it follows Linux behaviour and so that we don't accidentally break it in the future. Probably in src/test/socket/send_recv/test_send_recv.rs. We have some documentation here at https://shadow.github.io/docs/guide/writing_tests.html#system-call-tests.

(Also see #3526.)

magnified103 · 2025-03-13T09:48:15Z

This PR is ready for review.

I believe the new TCP implementation also has this issue. I attempted to fix it but couldn't write a test for the fix, as adding connect-new-tcp to the CMake config already causes existing tests to fail.

Additionally, I found a minor issue: calling connect() to a public IP on a socket bound to 127.0.0.1 returns EINVAL, but I believe it should return EISCONN. The EISCONN check is currently in the C code and needs to be moved out.

shadow/src/main/host/descriptor/tcp.c

Lines 1713 to 1716 in 3e478a9

    
           /* listening sockets can't connect */ 
        
           if (tcp_isValidListener(tcp)) { 
        
               return -EISCONN; 
        
           }

Do you want me to include the fix for that issue within this PR?

stevenengler

Thanks for the fixes and for the tests. This looks great! I just have one small change request, and then it's ready to merge.

src/test/socket/connect/test_connect.rs

src/test/socket/send_recv/test_send_recv.rs

stevenengler · 2025-03-23T04:36:38Z

I believe the new TCP implementation also has this issue. I attempted to fix it but couldn't write a test for the fix, as adding connect-new-tcp to the CMake config already causes existing tests to fail.

Don't worry about the new TCP implementation. It still needs lots of work, and once we eventually enable the tests, they should fail and we'll remember to fix this there too.

Additionally, I found a minor issue: calling connect() to a public IP on a socket bound to 127.0.0.1 returns EINVAL, but I believe it should return EISCONN. The EISCONN check is currently in the C code and needs to be moved out.

shadow/src/main/host/descriptor/tcp.c

Lines 1713 to 1716 in 3e478a9

/* listening sockets can't connect */

if (tcp_isValidListener(tcp)) {

return -EISCONN;

}

Do you want me to include the fix for that issue within this PR?

If there's a bug here, feel free to fix this either in this MR, or a new MR.

But I'm not clear on what the issue is here. Doesn't the new test_loopback_bound_connect test you added check this case? The test checks that this returns EINVAL for both TCP and UDP, both in shadow and outside shadow?

This PR brings a fix to this issue, although I think we'd want to mimic a routing table and perform these handling there.

Agreed. Hopefully we get there some day :)

src/main/host/descriptor/socket/inet/legacy_tcp.rs

stevenengler · 2025-03-30T18:52:47Z

I'm going to merge the PR as-is, and I'll do a follow up with the small changes. I hope you don't mind. I think this PR is good and I'd like to get this in to shadow.

magnified103 · 2025-03-30T18:55:41Z

I'm going to merge the PR as-is, and I'll do a follow up with the small changes. I hope you don't mind. I think this PR is good and I'd like to get this in to shadow.

Hi, sorry, I was a bit busy last week. I'll make the necessary changes today, and it'll be good.

stevenengler · 2025-03-30T19:03:26Z

I'm going to merge the PR as-is, and I'll do a follow up with the small changes. I hope you don't mind. I think this PR is good and I'd like to get this in to shadow.

Hi, sorry, I was a bit busy last week. I'll make the necessary changes today, and it'll be good.

Okay no problem, I don't mean to rush you. I just figured the changes I mentioned would be easy for me to add. But if you want to do them then that's great too, whenever you get time.

magnified103 · 2025-03-30T20:16:57Z

I've added another fix and test for loopback-bound listening sockets: sys_connect should return EISCONN regardless of whether the connecting address is loopback or not.

Can you review the changes I just made?

stevenengler

I've added another fix and test for loopback-bound listening sockets: sys_connect should return EISCONN regardless of whether the connecting address is loopback or not.

Can you review the changes I just made?

These look good, thanks!

Previously a UDP socket that called `sendto()` would be implicitly bound to an address on the same interface. So sending to 127.0.0.1 would cause the socket to be bound to 127.0.0.1. Sending to 1.2.3.4 would cause the socket to be bound to the public IP address of the host. This PR fixes the behaviour so that regardless of the address used with `sendto()`, the socket will always be bound to 0.0.0.0. This, along with #3531, should fix #3526.

magnified103 added 2 commits February 8, 2025 02:53

initial commie

9e87035

Merge branch 'temp_work'

8c06d40

github-actions bot assigned magnified103 Mar 12, 2025

github-actions bot added Component: Testing Unit and integration tests and frameworks Component: Main Composing the core Shadow executable labels Mar 12, 2025

magnified103 added 3 commits March 13, 2025 12:24

add tests, fix for inaddr_any

266419e

Merge branch 'temp_work' into fix-local-bound

4bfbbfa

add tests for sendto and sendmsg

ccf152e

magnified103 changed the title ~~Fix the issue where localhost-bounded sockets attempt to connect to an external address~~ Fix the issue where loopback-bounded sockets attempt to connect to an public IP Mar 13, 2025

magnified103 changed the title ~~Fix the issue where loopback-bounded sockets attempt to connect to an public IP~~ Fix the issue where loopback-bound sockets attempt to connect to an public IP Mar 13, 2025

magnified103 marked this pull request as ready for review March 13, 2025 09:20

magnified103 added 4 commits March 13, 2025 16:50

lint fix

1f44c79

Merge branch 'main' into fix-local-bound

2ddeae5

lint fix

9bbb6a2

Merge remote-tracking branch 'fork/fix-local-bound' into fix-local-bound

4b94198

magnified103 changed the title ~~Fix the issue where loopback-bound sockets attempt to connect to an public IP~~ Fix the issue where loopback-bound sockets attempt to connect to a public IP Mar 13, 2025

add fixes and tests

ab38ef7

stevenengler requested changes Mar 23, 2025

View reviewed changes

src/test/socket/connect/test_connect.rs Outdated Show resolved Hide resolved

src/test/socket/send_recv/test_send_recv.rs Outdated Show resolved Hide resolved

stevenengler reviewed Mar 23, 2025

View reviewed changes

src/main/host/descriptor/socket/inet/legacy_tcp.rs Show resolved Hide resolved

magnified103 added 3 commits March 31, 2025 02:43

change all addresses to 192.168.1.100

6efd198

fix the addresses and add some comments for clarity

52700ba

Merge remote-tracking branch 'origin/main' into fix-local-bound

4876514

add an entry to CHANGELOG

981fcc8

github-actions bot added the Component: Documentation In-repository documentation, under docs/ label Mar 30, 2025

magnified103 requested a review from stevenengler March 30, 2025 20:23

stevenengler approved these changes Mar 30, 2025

View reviewed changes

stevenengler merged commit 24343d0 into shadow:main Mar 30, 2025
24 of 25 checks passed

stevenengler mentioned this pull request Mar 31, 2025

Fix a bug with UDP implicit bind after sendto() #3545

Merged

stevenengler mentioned this pull request Mar 31, 2025

Shadow panics when sending a packet in CI test #3526

Closed

magnified103 deleted the fix-local-bound branch March 31, 2025 16:28

magnified103 mentioned this pull request Mar 31, 2025

Add routing table #3549

Open

	// a connected tcp socket must be bound
	let is_bound = unsafe { c::legacysocket_isBound(socket_ref.as_legacy_socket()) } == 1;
	if !is_bound {
	log::trace!("Implicitly binding listener socket");

	// implicit bind: bind to an ephemeral port (use default interface unless the remote

	// check if network reliability forces us to 'drop' the packet
	let reliability: f64 = Worker::with(\|w\| w.shared.reliability(src_ip, dst_ip).unwrap())
	.unwrap()

Conversation

magnified103 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magnified103 commented Mar 12, 2025

Uh oh!

stevenengler commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenengler commented Mar 12, 2025

Uh oh!

magnified103 commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenengler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stevenengler commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stevenengler commented Mar 30, 2025

Uh oh!

magnified103 commented Mar 30, 2025

Uh oh!

stevenengler commented Mar 30, 2025

Uh oh!

magnified103 commented Mar 30, 2025

Uh oh!

stevenengler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

magnified103 commented Mar 12, 2025 •

edited

Loading

stevenengler commented Mar 12, 2025 •

edited

Loading

magnified103 commented Mar 13, 2025 •

edited

Loading

stevenengler commented Mar 23, 2025 •

edited

Loading