johnyburd / net-route

Rust crate providing a cross platform interface for interacting with the routing table
https://docs.rs/net-route/
21 stars 14 forks source link

MacOS: Fix IPv6 routes #18

Closed gregor-netdebug closed 8 months ago

gregor-netdebug commented 9 months ago

An route message is a rt_msdhdr followed by a list of struct sockaddr. However, the sockaddr can be variable length (in particular because sockaddr_in6 is larger than sockaddr/sockaddr_in). This commit fixes the logic of walking the list of sockaddr's. Also did some refactoring to reduce the scope of some unsafe blocks.

Many of these chagnes are based on @adamierymenko's https://github.com/johnyburd/net-route/pull/4. However that PR had some bugs of its own.

The logic I'm using is inspired by https://opensource.apple.com/source/network_cmds/network_cmds-606.40.2/netstat.tproj/route.c.auto.html

gregor-netdebug commented 9 months ago

looks like I never actually opened a PR for this.

johnyburd commented 8 months ago

Thanks for the PR! I took a look and I'm seeing assert fails in the listen example.

Looking at this manpage it looks like maybe we should be decoding the first part of the header and then determining which header it actually is.

I tried changing the listen loop like this to test

let read = sock.read(&mut buf).await.expect("sock read err");
assert!(read > 0);
// NOTE: we don't know it's safe to read past type yet!
let hdr: &rt_msghdr = unsafe { mem::transmute(buf.as_mut_ptr()) };
if !matches!(hdr.rtm_type as u32, RTM_ADD | RTM_DELETE | RTM_CHANGE) {
    continue;
}
const HDR_SIZE: usize = mem::size_of::<rt_msghdr>();
assert!(read >= HDR_SIZE);
let route = message_to_route(hdr, &buf[HDR_SIZE..read]);

When I run the listen example with this code I see an error on this assert in message_to_route when decoding the RTAX_NETMASK socket address, specifically on RTM_DELETE messages.

assert!(buf.len() >= std::mem::size_of::<sockaddr>());

I'll keep poking at it as I get time but I figured I'd reply here in case you have a hint.

johnyburd commented 8 months ago

After some experimentation I was able to get something working that passes all my tests. I merged these changes plus some other fixes in another branch (see diff here). I'm still a little uncomfortable with it since I don't understand why the route message is the length it is, but I think it's still an improvement.

johnyburd commented 8 months ago

I'm going to go ahead and merge this with my tweaks in #19. If you have any more feedback please open another issue or PR. Thanks :)