-
Notifications
You must be signed in to change notification settings - Fork 16
Description
Hi,
While testing on macos I noticed that the events that are received from the route listen stream represent an attempt to add, delete or change a route. For example, I can try adding a route as a regular user, but even though it fails due to insufficient permissions I'm still able to see the event for this attempt in the event stream. Minimal repro:
use std::{sync::Arc, time::Duration};
use futures::StreamExt;
use net_route::{Handle, Route};
use tokio::time::sleep;
async fn f1(handle: Arc<Handle>) {
println!("f1> creating stream");
let stream = handle.route_listen_stream();
println!("f1> pinning stream");
futures::pin_mut!(stream);
while let Some(event) = stream.next().await {
println!("f1> event = {:?}", event);
}
}
async fn f2(handle: Arc<Handle>) {
let route = Route::new("10.11.12.13".parse().unwrap(), 32);
sleep(Duration::from_secs(3)).await; // just to let route listener spin up
let r1 = handle.add(&route.clone()).await;
println!("f2> add result = {:?}", r1);
sleep(Duration::from_secs(3)).await;
let r2 = handle.delete(&route.clone()).await;
println!("f2> delete result = {:?}", r2);
}
#[tokio::main]
async fn main() {
let handle = Arc::new(Handle::new().unwrap());
tokio::spawn(f1(handle.clone()));
tokio::spawn(f2(handle.clone()));
futures::pending!();
}
This looks like the expected behavior from the OS perspective. The header of the message obtained from pf route socket has rtm_errno field that indicates where the event was successful or note:
struct rt_msghdr {
u_short rmt_msglen; /* to skip over non-understood messages */
u_char rtm_version; /* future binary compatibility */
u_char rtm_type; /* message type */
u_short rmt_index; /* index for associated ifp */
pid_t rmt_pid; /* identify sender */
int rtm_addrs; /* bitmask identifying sockaddrs in msg */
int rtm_seq; /* for sender to identify action */
int rtm_errno; /* why failed */
int rtm_flags; /* flags, incl kern & message, e.g. DONE */
int rtm_use; /* from rtentry */
u_long rtm_inits; /* which values we are initializing */
struct rt_metrics rtm_rmx; /* metrics themselves */
};
Ref: https://leopard-adc.pepas.com/documentation/Darwin/Reference/ManPages/man4/route.4.html
net-route has access to this header and I think it should be taking it into consideration. Now the question arises as to what the code should do with this.
Option 1. if errno is not 0, then don't yield it into the stream here (https://github.com/johnyburd/net-route/blob/main/src/platform_impl/macos/macos.rs#L111-L114). This seems like the most naive way to fix, but at the same time, pf route exposes errno for a reason, and this may be useful for some users of net-route.
Option 2: expose errno to the code using net-route and let it decide how it wants to handle it. Here we can add errno (possibly with a more descriptive name) into Route struct. This is suboptimal imo as errno doesn't necessary belong to the Route itself. But this way allows to keep it backwards compatible.
Option 3: similar to option 2, but expand RouteChange (https://github.com/johnyburd/net-route/blob/main/src/lib.rs#L217-L221) enum to include errno, e.g.
pub enum RouteChange {
Add(Route, Result<(), Error>),
Delete(Route, Result<(), Error>),
Change(Route, Result<(), Error>),
}
or with generic metadata field
struct Metadata {
seq: i32,
errno: i32,
}
pub enum RouteChange {
Add(Route, Metadata),
Delete(Route, Metadata),
Change(Route, Metadata),
}
I'm leaning towards 3rd option, although it is a breaking change. What do you think about this?
Also, it would be great if someone with Windows experience could speak on how this works on Windows.