Add split tunneling for macOS #5844
Conversation
|
Yay, great to see this feature finally being worked on. |
dlon
force-pushed
the
macos-add-split-tunnel
branch
2 times, most recently
from
2e56256 to
439bd5b
Compare
There was a problem hiding this comment.
Reviewed 31 of 37 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 32 of 37 files reviewed, 31 unresolved discussions (waiting on @dlon)
talpid-core/src/firewall/macos.rs line 153 at r2 (raw file):
self.get_allow_tunnel_rules(&tunnel.interface, allowed_tunnel_traffic)?, ); }
Nit Over longer code blocks, I prefer to use match over if let .. { .. } else { .. }. Do as you wish with that 😊
if let Some(tunnel) = tunnel {
match redirct_interface {
Some(redirect_interface) => {
try_enable_forwarding();
if !allowed_tunnel_traffic.all() {
log::warn!("Split tunneling does not respect the 'allowed tunnel traffic' setting");
}
rules.extend(self.get_allow_established_tunnel_rules(
&tunnel.interface,
allowed_tunnel_traffic,
)?);
rules.append(
&mut self.get_split_tunnel_rules(
&tunnel.interface,
redirect_interface,
)?,
);
}
None => {
rules.extend(self.get_allow_tunnel_rules(
&tunnel.interface,
allowed_tunnel_traffic,
)?);
}
}
}Code quote:
if let Some(redirect_interface) = redirect_interface {
try_enable_forwarding();
if !allowed_tunnel_traffic.all() {
log::warn!("Split tunneling does not respect the 'allowed tunnel traffic' setting");
}
rules.extend(self.get_allow_established_tunnel_rules(
&tunnel.interface,
allowed_tunnel_traffic,
)?);
rules.append(
&mut self
.get_split_tunnel_rules(&tunnel.interface, redirect_interface)?,
);
} else {
rules.extend(
self.get_allow_tunnel_rules(&tunnel.interface, allowed_tunnel_traffic)?,
);
}talpid-core/src/firewall/macos.rs line 398 at r2 (raw file):
tunnel_interface: &str, allowed_traffic: &AllowedTunnelTraffic, get_tcp_flags: impl FnOnce() -> pfctl::TcpFlags,
Why is get_tcp_flags a closure rather than pfctl::TcpFlags directly? 😊
fn get_allow_tunnel_rules(
&self,
tunnel_interface: &str,
allowed_traffic: &AllowedTunnelTraffic,
) -> Result<Vec<pfctl::FilterRule>> {
self.get_allow_tunnel_rules_inner(tunnel_interface, allowed_traffic, Self::get_tcp_flags())
}
fn get_allow_established_tunnel_rules(
&self,
tunnel_interface: &str,
allowed_traffic: &AllowedTunnelTraffic,
) -> Result<Vec<pfctl::FilterRule>> {
self.get_allow_tunnel_rules_inner(tunnel_interface, allowed_traffic,
pfctl::TcpFlags::new(
&[pfctl::TcpFlag::Syn, pfctl::TcpFlag::Ack],
&[pfctl::TcpFlag::Syn, pfctl::TcpFlag::Ack],
)
)
}
fn get_allow_tunnel_rules_inner(
&self,
tunnel_interface: &str,
allowed_traffic: &AllowedTunnelTraffic,
get_tcp_flags: pfctl::TcpFlags,
) -> Result<Vec<pfctl::FilterRule>> { .. }Code quote:
get_tcp_flags: impl FnOnce() -> pfctl::TcpFlags,talpid-core/src/firewall/macos.rs line 744 at r2 (raw file):
} fn try_enable_forwarding() {
try_enable_forwarding implies that the function could fail, but this is not reflected in the type of the function. My suggestion is to either propagate a Result<()> or simply change the name to enable_forwarding 😊
Code quote:
fn try_enable_forwarding()talpid-core/src/firewall/macos.rs line 768 at r2 (raw file):
}; let result = unsafe {
A note about the safety would be nice 😊
Code quote:
let result = unsafe {talpid-core/src/split_tunnel/macos/generate-bindings.sh line 13 at r2 (raw file):
curl https://opensource.apple.com/source/xnu/xnu-3789.41.3/bsd/net/pktap.h -o include/pktap.h curl https://opensource.apple.com/source/libpcap/libpcap-67/libpcap/pcap/pcap.h -o include/pcap.h curl https://opensource.apple.com/source/xnu/xnu-3789.41.3/bsd/net/bpf.h -o include/bpf.h
Is it a good idea to issue curl to grab the header files? Wouldn't it be nice if we could sign them? 😊
Code quote:
curl https://opensource.apple.com/source/xnu/xnu-3789.41.3/bsd/net/pktap.h -o include/pktap.h
curl https://opensource.apple.com/source/libpcap/libpcap-67/libpcap/pcap/pcap.h -o include/pcap.h
curl https://opensource.apple.com/source/xnu/xnu-3789.41.3/bsd/net/bpf.h -o include/bpf.htalpid-core/src/split_tunnel/macos/mod.rs line 38 at r2 (raw file):
vpn_interface: Option<VpnInterface>, }, HasProcessMonitor {
Nit: HasProccessMonitor is a bit vague - what is the implication for the state of split tunneling? In the same vein as the NoExclusions-variant, could this variant be named something like Excluding? 😊
If the name is adequate as is, it would be nice with a doc-comment explaining the intent/life-cycle of each State-variant 😊
Code quote:
HasProcessMonitortalpid-core/src/split_tunnel/macos/mod.rs line 95 at r2 (raw file):
} log::debug!("Initializing process monitor");
Instead of returning early here, split this into two match-branches:
match &mut self.state {
State::NoExclusions { .. } if paths.is_empty() => Ok(()), // Maybe we should log here? Maybe not
State::NoExclusions { .. } => {
log::debug!("Initializing process monitor");
..
}
..
}Code quote:
match &mut self.state {
State::NoExclusions { .. } => {
if paths.is_empty() {
return Ok(());
}
log::debug!("Initializing process monitor");talpid-core/src/split_tunnel/macos/mod.rs line 105 at r2 (raw file):
unreachable!("unexpected state") }; let process = process::ProcessMonitor::spawn().await?;
This dance with std::mem::replace is a bit hard to follow, but it should be simple to re-structure the error handling a bit! It would be nice to avoid the unreachable! branch as well. What do you think about the following code?
match &mut self.state {
State::NoExclusions { route_manager, vpn_interface } => {
log::debug!("Initializing process monitor");
match process::ProcessMonitor::spawn() {
Err(err) => {
// It is probably a good idea to log the error here
*self.state = State::Failed;
}
Ok(process) => {
process.states().exclude_paths(paths);
self.state = State::HasProcessMonitor {
route_manager,
process,
};
self.set_tunnel(vpn_interface).await
}
}
}
..
} Code quote:
let prev_state = std::mem::replace(&mut self.state, State::Failed);
let State::NoExclusions {
route_manager,
vpn_interface,
} = prev_state
else {
unreachable!("unexpected state")
};
let process = process::ProcessMonitor::spawn().await?;talpid-core/src/split_tunnel/macos/mod.rs line 158 at r2 (raw file):
} } }
I'll make the same argument here: I think the use of std::mem::replace is needlessly complex, and it would be nice to avoid the use of unreachable! in this case 😊
Code quote:
State::Initialized { .. } => {
let prev_state = std::mem::replace(&mut self.state, State::Failed);
let State::Initialized {
mut process,
tun_handle,
} = prev_state
else {
unreachable!("unexpected state")
};
log::debug!("Updating split tunnel device");
match tun_handle.set_vpn_tunnel(new_vpn_interface).await {
Ok(tun_handle) => {
self.state = State::Initialized {
process,
tun_handle,
};
Ok(())
}
Err(error) => {
process.shutdown().await;
Err(error.into())
}
}
}talpid-core/src/split_tunnel/macos/mod.rs line 162 at r2 (raw file):
if new_vpn_interface.is_none() { return Ok(()); }
Split this condition check + early return into a separate branch, see above comment about the similar change suggestion 😊
Code quote:
State::HasProcessMonitor { .. } => {
if new_vpn_interface.is_none() {
return Ok(());
}talpid-core/src/split_tunnel/macos/mod.rs line 170 at r2 (raw file):
else { unreachable!("unexpected state"); };
See above comment about error handling with std::mem::replace 😄
Code quote:
let State::HasProcessMonitor {
route_manager,
mut process,
} = std::mem::replace(&mut self.state, State::Failed)
else {
unreachable!("unexpected state");
};talpid-core/src/split_tunnel/macos/mod.rs line 181 at r2 (raw file):
ExclusionStatus::Included => tun::RoutingDecision::VpnTunnel, ExclusionStatus::Unknown => { // TODO: Delay decision until next exec
TODO
Code quote:
// TODO: Delay decision until next exectalpid-core/src/split_tunnel/macos/include/bindings.h line 9 at r2 (raw file):
// included header is missing "want_pktap" //#include <pcap/pcap.h>
Comment
Code quote:
//#include <pcap/pcap.h>talpid-core/src/tunnel_state_machine/connected_state.rs line 156 at r2 (raw file):
#[cfg(target_os = "macos")] let redirect_interface = shared_values.split_tunnel.interface().map(|s| s.to_owned());
Nit: Call cloned instead of mapping to_owned 😊
let redirect_interface = shared_values.split_tunnel.interface().cloned();Code quote:
let redirect_interface = shared_values.split_tunnel.interface().map(|s| s.to_owned());talpid-core/src/tunnel_state_machine/connected_state.rs line 335 at r2 (raw file):
#[cfg(target_os = "windows")] shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")]
Nit: The amount of visual noise from all of the cfg directives is bothering me 😅
Could we reduce it somewhat by scoping them per match-branch? Something like this
#[cfg(target_os = "windows")]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => { windows-specific code }
#[cfg(target_os = "macos")]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => { macos-specific code }Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]talpid-core/src/tunnel_state_machine/connecting_state.rs line 162 at r2 (raw file):
#[cfg(target_os = "macos")] let redirect_interface = shared_values.split_tunnel.interface().map(|s| s.to_owned());
Nit: dito
Code quote:
let redirect_interface = shared_values.split_tunnel.interface().map(|s| s.to_owned());talpid-core/src/tunnel_state_machine/connecting_state.rs line 494 at r2 (raw file):
shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")] {
Nit: Dito
Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]
{talpid-core/src/tunnel_state_machine/connecting_state.rs line 523 at r2 (raw file):
} } }
It feels like the error handling is quite similar to equivalent branch in talpid-core/src/tunnel_state_machine/connected_state.rs - if they are identical, would there be some way of handling them in a unified way? Otherwise I'm fine with not blocking on DRY, but if the pattern is complex enough I'm often inclined to abstract it just to be able to put a label on it and/or document it 😊
Code quote:
match shared_values.set_exclude_paths(paths) {
Ok(added_device) => {
let _ = result_tx.send(Ok(()));
if added_device {
if let Err(error) = Self::set_firewall_policy(
shared_values,
&self.tunnel_parameters,
&self.tunnel_metadata,
self.allowed_tunnel_traffic.clone(),
) {
return self.disconnect(
shared_values,
AfterDisconnect::Block(
ErrorStateCause::SetFirewallPolicyError(error),
),
);
}
}
}
Err(error) => {
let _ = result_tx.send(Err(error));
return self.disconnect(
shared_values,
AfterDisconnect::Block(ErrorStateCause::SplitTunnelError),
);
}
}
}talpid-core/src/tunnel_state_machine/disconnected_state.rs line 222 at r2 (raw file):
shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")] let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));
Dito
Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]
let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));talpid-core/src/tunnel_state_machine/disconnecting_state.rs line 83 at r2 (raw file):
shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")] let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));
Dito
Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]
let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));talpid-core/src/tunnel_state_machine/disconnecting_state.rs line 132 at r2 (raw file):
shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")] let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));
Dito
Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]
let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));talpid-core/src/tunnel_state_machine/disconnecting_state.rs line 182 at r2 (raw file):
shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")] let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));
Dito
Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]
let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));talpid-core/src/tunnel_state_machine/error_state.rs line 219 at r2 (raw file):
shared_values.split_tunnel.set_paths(&paths, result_tx); #[cfg(target_os = "macos")] let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));
Dito
Code quote:
#[cfg(any(target_os = "windows", target_os = "macos"))]
Some(TunnelCommand::SetExcludedApps(result_tx, paths)) => {
#[cfg(target_os = "windows")]
shared_values.split_tunnel.set_paths(&paths, result_tx);
#[cfg(target_os = "macos")]
let _ = result_tx.send(shared_values.set_exclude_paths(paths).map(|_| ()));talpid-core/src/tunnel_state_machine/mod.rs line 368 at r2 (raw file):
.exclude_paths .iter() .map(|p| p.into())
Nit: Prefer to specify the type explicitly instead of using into:
.map(TargetType::from)Code quote:
.map(|p| p.into())talpid-core/src/tunnel_state_machine/mod.rs line 501 at r2 (raw file):
if let Err(error) = self.runtime.block_on( self.split_tunnel .set_exclude_paths(paths.into_iter().map(|p| p.into()).collect()),
It would be nice to spell out the target type here:
map(TargetType::from)Code quote:
map(|p| p.into())talpid-core/src/tunnel_state_machine/mod.rs line 508 at r2 (raw file):
); return Err(error); }
Nit: use map_err and ? to propagate the error 😊
Code quote:
if let Err(error) = self.runtime.block_on(
self.split_tunnel
.set_exclude_paths(paths.into_iter().map(|p| p.into()).collect()),
) {
log::error!(
"{}",
error.display_chain_with_msg("Failed to set split tunnel paths")
);
return Err(error);
}talpid-core/src/tunnel_state_machine/mod.rs line 510 at r2 (raw file):
} let has_interface = self.split_tunnel.interface().is_some(); // return whether an ST utun was created
Promote this to a doc-comment
Code quote:
// return whether an ST utun was createdtalpid-core/src/tunnel_state_machine/mod.rs line 515 at r2 (raw file):
#[cfg(target_os = "macos")] pub fn maybe_enable_split_tunnel(
Nit: Why is this function call maybe_? Is it because it is fallible? if there is no equivalent infallible function, I would suggest that we drop the maybe_-prefix since the return type is a Result 😊
Code quote:
pub fn maybe_enable_split_tunneltalpid-core/src/tunnel_state_machine/mod.rs line 549 at r2 (raw file):
); return Err(ErrorStateCause::SplitTunnelError); }
Nit: use map_err and propagate the error with ? 😊
self.runtime
.block_on(self.split_tunnel.set_tunnel(Some(vpn_interface)))
.map_err(|error| {
log::error!("{}", error.display_chain_with_msg("Failed to set VPN interface for split tunnel"));
ErrorStateCause::SplitTunnelError
})?;Code quote:
if let Err(error) = self
.runtime
.block_on(self.split_tunnel.set_tunnel(Some(vpn_interface)))
{
log::error!(
"{}",
error.display_chain_with_msg("Failed to set VPN interface for split tunnel")
);
return Err(ErrorStateCause::SplitTunnelError);
}talpid-routing/src/lib.rs line 40 at r2 (raw file):
self.0 } }
Missing #[cfg(target_os = "macos")] - cargo check fails on Linux 😢
Code quote:
impl MacAddress {
/// Consume bytes that make up the link address
pub fn into_bytes(self) -> [u8; 6] {
self.0
}
}talpid-routing/src/unix/macos/interface.rs line 91 at r2 (raw file):
Family::V6.default_network() }; let router_addr = SocketAddr::from((route.router_ip, 0));
Would be nice with a small comment explaining the use of port 0 😊
// Use an available random port
let router_addr = SocketAddr::from((router.router_ip, 0));Code quote:
let router_addr = SocketAddr::from((route.router_ip, 0));
There was a problem hiding this comment.
Reviewable status: 32 of 37 files reviewed, 51 unresolved discussions (waiting on @dlon)
talpid-core/src/split_tunnel/macos/bpf.rs line 77 at r2 (raw file):
let dup = self.file.try_clone().unwrap(); (ReadHalf(dup), WriteHalf(self.file)) }
Return a Result<(ReadHalf, WriteHalf), Error> instead of calling unwrap
Code quote:
pub fn split(self) -> (ReadHalf, WriteHalf) {
let dup = self.file.try_clone().unwrap();
(ReadHalf(dup), WriteHalf(self.file))
}talpid-core/src/split_tunnel/macos/bpf.rs line 100 at r2 (raw file):
} return Err(Error::OpenBpfDevice(error)); }
Nit: Split up into multiple match-branches
// Find a free bpf device
for dev_num in 0..MAX_BPF_COUNT {
// Open as O_RDWR
match File::options()
.read(true)
.write(true)
.open(format!("/dev/bpf{dev_num}"))
{
Err(error) if error.raw_os_error() == Some(EBUSY) => continue,
Err(error) => return Err(Error::OpenBpfDevice(error)),
Ok(file) => {
log::trace!("Opened BPF device: /dev/bpf{dev_num}");
return Ok(file);
}
}
}Code quote:
Err(error) => {
if error.raw_os_error() == Some(EBUSY) {
// This BPF device is in use
continue;
}
return Err(Error::OpenBpfDevice(error));
}talpid-core/src/split_tunnel/macos/bpf.rs line 107 at r2 (raw file):
pub fn set_nonblocking(&self, enabled: bool) -> Result<(), Error> { let mut flags = unsafe { libc::fcntl(self.as_raw_fd(), F_GETFL) };
Would be nice to add a comment regarding the use of unsafe
Code quote:
let mut flags = unsafe { libc::fcntl(self.as_raw_fd(), F_GETFL) };talpid-core/src/split_tunnel/macos/bpf.rs line 117 at r2 (raw file):
} let result = unsafe { libc::fcntl(self.as_raw_fd(), F_SETFL, flags) };
Would be nice to add a comment regarding the use of unsafe
Code quote:
let result = unsafe { libc::fcntl(self.as_raw_fd(), F_SETFL, flags) };talpid-core/src/split_tunnel/macos/bpf.rs line 126 at r2 (raw file):
pub fn get_stats(&self) -> Result<Stats, Error> { let mut raw_stats: bpf_stat = unsafe { mem::zeroed() }; unsafe { ioctl!(self.file.as_raw_fd(), BIOCGSTATS, &mut raw_stats) }?;
Would be nice to add a comment regarding the use of unsafe
Code quote:
let mut raw_stats: bpf_stat = unsafe { mem::zeroed() };
unsafe { ioctl!(self.file.as_raw_fd(), BIOCGSTATS, &mut raw_stats) }?;talpid-core/src/split_tunnel/macos/bpf.rs line 136 at r2 (raw file):
/// Set BIOCSETIF pub fn set_interface(&self, name: &str) -> Result<(), Error> { let mut ifr: ifreq = unsafe { std::mem::zeroed() };
Would be nice to add a comment regarding the use of unsafe
Code quote:
let mut ifr: ifreq = unsafe { std::mem::zeroed() };talpid-core/src/split_tunnel/macos/bpf.rs line 150 at r2 (raw file):
); ioctl!(self.file.as_raw_fd(), BIOCSETIF, &ifr) }
Would be nice to add a comment regarding the use of unsafe
Code quote:
unsafe {
std::ptr::copy_nonoverlapping(
name_bytes.as_ptr(),
&mut ifr.ifr_name as *mut _ as *mut _,
name_bytes.len(),
);
ioctl!(self.file.as_raw_fd(), BIOCSETIF, &ifr)
}talpid-core/src/split_tunnel/macos/bpf.rs line 156 at r2 (raw file):
pub fn set_immediate(&self, enable: bool) -> Result<(), Error> { let enable: c_int = if enable { 1 } else { 0 }; unsafe { ioctl!(self.file.as_raw_fd(), BIOCIMMEDIATE, &enable) }
You get the point
Code quote:
unsafe { ioctl!(self.file.as_raw_fd(), BIOCIMMEDIATE, &enable) }talpid-core/src/split_tunnel/macos/bpf.rs line 162 at r2 (raw file):
pub fn set_see_sent(&self, enable: bool) -> Result<(), Error> { let enable: c_int = if enable { 1 } else { 0 }; unsafe { ioctl!(self.file.as_raw_fd(), BIOCSSEESENT, &enable) }
unsafe without safety reasoning
Code quote:
unsafe { ioctl!(self.file.as_raw_fd(), BIOCSSEESENT, &enable) }talpid-core/src/split_tunnel/macos/bpf.rs line 168 at r2 (raw file):
pub fn set_header_complete(&self, enable: bool) -> Result<(), Error> { let enable: c_int = if enable { 1 } else { 0 }; unsafe { ioctl!(self.file.as_raw_fd(), BIOCSHDRCMPLT, &enable) }
unsafe without safety reasoning
Code quote:
unsafe { ioctl!(self.file.as_raw_fd(), BIOCSHDRCMPLT, &enable) }talpid-core/src/split_tunnel/macos/bpf.rs line 173 at r2 (raw file):
pub fn set_want_pktap(&self, enable: bool) -> Result<(), Error> { let enable: c_int = if enable { 1 } else { 0 }; unsafe { ioctl!(self.file.as_raw_fd(), BIOCSWANTPKTAP, &enable) }
unsafe without safety reasoning
Code quote:
unsafe { ioctl!(self.file.as_raw_fd(), BIOCSWANTPKTAP, &enable) }talpid-core/src/split_tunnel/macos/bpf.rs line 179 at r2 (raw file):
unsafe { ioctl!(self.file.as_raw_fd(), BIOCSBLEN, &mut buffer_size)?; }
unsafe without safety reasoning
Code quote:
unsafe {
ioctl!(self.file.as_raw_fd(), BIOCSBLEN, &mut buffer_size)?;
}talpid-core/src/split_tunnel/macos/bpf.rs line 187 at r2 (raw file):
unsafe { ioctl!(self.file.as_raw_fd(), BIOCGBLEN, &mut buf_size)?; }
unsafe without safety reasoning
Code quote:
unsafe {
ioctl!(self.file.as_raw_fd(), BIOCGBLEN, &mut buf_size)?;
}talpid-core/src/split_tunnel/macos/bpf.rs line 195 at r2 (raw file):
unsafe { ioctl!(self.file.as_raw_fd(), BIOCGDLT, &mut dlt)?; }
unsafe without safety reasoning
Code quote:
unsafe {
ioctl!(self.file.as_raw_fd(), BIOCGDLT, &mut dlt)?;
}talpid-core/src/split_tunnel/macos/bpf.rs line 219 at r2 (raw file):
fn flush(&mut self) -> io::Result<()> { // no-op // TODO: verify
TODO
Code quote:
// TODO: verifytalpid-core/src/split_tunnel/macos/bpf.rs line 231 at r2 (raw file):
fn flush(&mut self) -> io::Result<()> { // no-op // TODO: verify
TODO
Code quote:
// TODO: verifytalpid-core/src/split_tunnel/macos/bpf.rs line 249 at r2 (raw file):
fn flush(&mut self) -> io::Result<()> { // no-op // TODO: verify
TODO
Code quote:
// TODO: verifytalpid-core/src/split_tunnel/macos/bpf.rs line 301 at r2 (raw file):
if data.len() < mem::size_of::<bpf_hdr>() { return None; }
Is it obvious for the caller that parse_bpf_header returns None if data.len() < size_of(bpf_hdr)? I think it deserves a mention in a doc-comment for this function 😊
Code quote:
pub fn parse_bpf_header<'a>(data: &'a [u8]) -> Option<(&'a bpf_hdr, usize)> {
if data.len() < mem::size_of::<bpf_hdr>() {
return None;
}talpid-core/src/split_tunnel/macos/bpf.rs line 302 at r2 (raw file):
return None; } let bpf_header: &bpf_hdr = unsafe { &*(data.as_ptr() as *const u8 as *const bpf_hdr) };
unsafe without safety reasoning
Code quote:
let bpf_header: &bpf_hdr = unsafe { &*(data.as_ptr() as *const u8 as *const bpf_hdr) };talpid-core/src/split_tunnel/macos/bpf.rs line 312 at r2 (raw file):
const ALIGNMENT: u32 = BPF_ALIGNMENT as u32; return (n + (ALIGNMENT - 1)) & (!(ALIGNMENT - 1)); }
Nit A bit esoteric for people not familiar with bpf - would it be okay to add a doc-comment? If it is overkill, skip it 😊
Code quote:
const fn bpf_wordalign(n: u32) -> u32 {
const ALIGNMENT: u32 = BPF_ALIGNMENT as u32;
return (n + (ALIGNMENT - 1)) & (!(ALIGNMENT - 1));
}
There was a problem hiding this comment.
Reviewed 1 of 37 files at r1.
Reviewable status: 33 of 37 files reviewed, 65 unresolved discussions (waiting on @dlon)
talpid-core/src/split_tunnel/macos/process.rs line 48 at r2 (raw file):
let excluded_paths = vec![]; states.exclude_paths(excluded_paths);
Nit inline excluded_paths, or skip calling exclude_paths entirely if possible 😊
Code quote:
let excluded_paths = vec![];
states.exclude_paths(excluded_paths);talpid-core/src/split_tunnel/macos/process.rs line 134 at r2 (raw file):
processes: HashMap<u32, ProcessInfo>, exclude_paths: Vec<PathBuf>, }
Is there a particular reason why exclude_paths is not a HashSet? 😊
Code quote:
#[derive(Debug)]
struct InnerProcessStates {
processes: HashMap<u32, ProcessInfo>,
exclude_paths: Vec<PathBuf>,
}talpid-core/src/split_tunnel/macos/process.rs line 148 at r2 (raw file):
for pid in procs { let path = process_path(pid).map_err(|error| Error::FindProcessPath(error, pid))?; let ppid = process_info(pid)
ppid is a bit of an obscure name for parent_pid imho 😊
Code quote:
let ppid =talpid-core/src/split_tunnel/macos/process.rs line 153 at r2 (raw file):
log::error!("Failed to obtain parent pid for {pid}: {error}"); 0 });
Is it valid to say that the parent PID is 0 if a real parent PID can not be found? I guess that the init process has to be the parent in that case, and if that is the intention here it would be nice with a small comment on what is meant with PID 0 😊
Code quote:
.unwrap_or_else(|error| {
log::error!("Failed to obtain parent pid for {pid}: {error}");
0
});talpid-core/src/split_tunnel/macos/process.rs line 175 at r2 (raw file):
.filter(|old_path| paths.contains(old_path)) .cloned() .collect();
If InnerProcessStates.excluded_paths was modeled as a set, this would be the intersection between self.excluded_paths and paths, right? 😊
It would kind of force the paths argument to be either impl IntoIterator<Item = PathBuf> or a HashSet<PathBuf>, but I think it could simplify some of the code in this function a bit 😊
Code quote:
// Remove no-longer excluded paths from exclusion list
let mut new_exclude_paths: Vec<_> = info
.excluded_by_paths
.iter()
.filter(|old_path| paths.contains(old_path))
.cloned()
.collect();talpid-core/src/split_tunnel/macos/process.rs line 252 at r2 (raw file):
if info.excluded_by_paths.contains(&info.exec_path) { return; }
Do we return early here because process corresponding to pid is already excluded? Would be nice with a comment 😊
Code quote:
if info.excluded_by_paths.contains(&info.exec_path) {
return;
}talpid-core/src/split_tunnel/macos/process.rs line 273 at r2 (raw file):
/// Obtain a list of all pids fn list_pids() -> io::Result<Vec<u32>> { let num_pids = unsafe { proc_listallpids(ptr::null_mut(), 0) };
Unsafe with safety argument :(
Code quote:
let num_pids = unsafe { proc_listallpids(ptr::null_mut(), 0) };talpid-core/src/split_tunnel/macos/process.rs line 279 at r2 (raw file):
let mut pids = vec![0u32; usize::try_from(num_pids).unwrap()]; let buf_sz = (u32::BITS as usize / 8 * (num_pids as usize)) as i32;
Isn't this the same as std::mem::size_of::<u32>()? 😊
If your are okay with it, I would suggest writing this piece of code as
let buf_sz = num_pids as usize * std::mem::size_of::<u32>();
let num_pids = unsafe { proc_listallpids(pids.as_mut_ptr() as *mut c_void, buf_sz as i32) };Code quote:
u32::BITS as usize / 8talpid-core/src/split_tunnel/macos/process.rs line 280 at r2 (raw file):
let buf_sz = (u32::BITS as usize / 8 * (num_pids as usize)) as i32; let num_pids = unsafe { proc_listallpids(pids.as_mut_ptr() as *mut c_void, buf_sz) };
Unsafe with safety argument :(
Code quote:
let num_pids = unsafe { proc_listallpids(pids.as_mut_ptr() as *mut c_void, buf_sz) };talpid-core/src/split_tunnel/macos/process.rs line 309 at r2 (raw file):
fn process_info(pid: u32) -> io::Result<proc_bsdinfo> { let mut info: proc_bsdinfo = unsafe { std::mem::zeroed() };
Unsafe without safety argument :(
Code quote:
let mut info: proc_bsdinfo = unsafe { std::mem::zeroed() };talpid-core/src/split_tunnel/macos/process.rs line 319 at r2 (raw file):
mem::size_of::<proc_bsdinfo>() as i32, ) };
Unsafe without safety argument :(
Code quote:
let result = unsafe {
proc_pidinfo(
pid as i32,
PROC_PIDTBSDINFO,
0,
&mut info as *mut _ as *mut c_void,
mem::size_of::<proc_bsdinfo>() as i32,
)
};talpid-core/src/split_tunnel/macos/process.rs line 330 at r2 (raw file):
struct ProcessInfo { exec_path: PathBuf, ppid: u32,
nit: A bit obscure naming - I would prefer parent_pid (or something along those lines) 😊
Code quote:
ppid: u32,talpid-core/src/split_tunnel/macos/process.rs line 331 at r2 (raw file):
exec_path: PathBuf, ppid: u32, // inherited: bool,
Dead code
Code quote:
// inherited: bool,talpid-core/src/split_tunnel/macos/process.rs line 394 at r2 (raw file):
event: ESEvent, process: ESProcess, }
Nit For non-mac programms, the ES-prefix seems a bit strange. To decrease the overhead of understanding this code regardless of prior experience, it would be nice to have a doc-comment for each struct (and preferably each field)
Code quote:
#[derive(Debug, Deserialize)]
struct ESForkChild {
audit_token: ESAuditToken,
}
#[derive(Debug, Deserialize)]
struct ESForkEvent {
child: ESForkChild,
}
#[derive(Debug, Deserialize)]
struct ESExecEvent {
dyld_exec_path: String,
}
#[derive(Debug, Deserialize)]
#[serde(rename_all = "lowercase")]
enum ESEvent {
Fork(ESForkEvent),
Exec(ESExecEvent),
Exit(serde_json::Value),
}
#[derive(Debug, Deserialize)]
struct ESExecutable {
path: PathBuf,
}
#[derive(Debug, Deserialize)]
struct ESAuditToken {
pid: u32,
}
#[derive(Debug, Deserialize)]
struct ESProcess {
audit_token: ESAuditToken,
executable: ESExecutable,
}
#[derive(Debug, Deserialize)]
struct ESMessage {
event: ESEvent,
process: ESProcess,
}
|
docs/split-tunneling.md should be updated as well 🚀 |
There was a problem hiding this comment.
Reviewable status: 27 of 38 files reviewed, 65 unresolved discussions (waiting on @MarkusPettersson98)
talpid-core/src/split_tunnel/macos/mod.rs line 95 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Instead of returning early here, split this into two match-branches:
match &mut self.state { State::NoExclusions { .. } if paths.is_empty() => Ok(()), // Maybe we should log here? Maybe not State::NoExclusions { .. } => { log::debug!("Initializing process monitor"); .. } .. }
Done.
talpid-core/src/split_tunnel/macos/mod.rs line 105 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
This dance with
std::mem::replaceis a bit harder to follow then to re-structure the error handling a bit. It would be nice to avoid theunreachable!branch as well. What do you think about the following code?match &mut self.state { State::NoExclusions { route_manager, vpn_interface } => { log::debug!("Initializing process monitor"); match process::ProcessMonitor::spawn() { Err(err) => { // It is probably a good idea to log the error here *self.state = State::Failed; } Ok(process) => { process.states().exclude_paths(paths); self.state = State::HasProcessMonitor { route_manager, process, }; self.set_tunnel(vpn_interface).await } } } .. }
Doesn't work, as discussed. But this should definitely be improved.
talpid-core/src/split_tunnel/macos/mod.rs line 158 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
I'll make the same argument here: I think the use of
std::mem::replaceis needlessly complex, and it would be nice to avoid the use ofunreachable!in this case 😊
See above.
talpid-core/src/split_tunnel/macos/mod.rs line 162 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Split this condition check + early return into a separate branch, see above comment about the similar change suggestion 😊
Done.
talpid-core/src/split_tunnel/macos/mod.rs line 170 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
See above comment about error handling with
std::mem::replace😄
See above.
talpid-core/src/split_tunnel/macos/process.rs line 153 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Is it valid to say that the parent PID is 0 if a real parent PID can not be found? I guess that the init process has to be the parent in that case, and if that is the intention here it would be nice with a small comment on what is meant with PID 0 😊
That's a good question. Arguably, the grandparent is the real new parent, but I'm not sure what the technically correct answer is.
talpid-core/src/split_tunnel/macos/process.rs line 175 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
If
InnerProcessStates.excluded_pathswas modeled as a set, this would be the intersection betweenself.excluded_pathsandpaths, right? 😊
It would kind of force thepathsargument to be eitherimpl IntoIterator<Item = PathBuf>or aHashSet<PathBuf>, but I think it could simplify some of the code in this function a bit 😊
Yep. I'll switch to using sets.
talpid-core/src/split_tunnel/macos/process.rs line 279 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Isn't this the same as
std::mem::size_of::<u32>()? 😊If your are okay with it, I would suggest writing this piece of code as
let buf_sz = num_pids as usize * std::mem::size_of::<u32>(); let num_pids = unsafe { proc_listallpids(pids.as_mut_ptr() as *mut c_void, buf_sz as i32) };
Yes. That was a bit silly.
talpid-core/src/tunnel_state_machine/connected_state.rs line 156 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Nit: Call
clonedinstead of mappingto_owned😊let redirect_interface = shared_values.split_tunnel.interface().cloned();
Thanks.
talpid-core/src/tunnel_state_machine/mod.rs line 368 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Nit: Prefer to specify the type explicitly instead of using
into:.map(TargetType::from)
Agreed!
talpid-core/src/tunnel_state_machine/mod.rs line 501 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
It would be nice to spell out the target type here:
map(TargetType::from)
Agreed!
talpid-core/src/tunnel_state_machine/mod.rs line 515 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Nit: Why is this function call
maybe_? Is it because it is fallible? if there is no equivalent infallible function, I would suggest that we drop themaybe_-prefix since the return type is aResult😊
The maybe_ prefix is because it only starts the machinery if
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
a10eadd to
6ce8e6c
Compare
There was a problem hiding this comment.
Reviewable status: 22 of 38 files reviewed, 53 unresolved discussions (waiting on @MarkusPettersson98)
talpid-core/src/firewall/macos.rs line 398 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Why is
get_tcp_flagsa closure rather thanpfctl::TcpFlagsdirectly? 😊fn get_allow_tunnel_rules( &self, tunnel_interface: &str, allowed_traffic: &AllowedTunnelTraffic, ) -> Result<Vec<pfctl::FilterRule>> { self.get_allow_tunnel_rules_inner(tunnel_interface, allowed_traffic, Self::get_tcp_flags()) } fn get_allow_established_tunnel_rules( &self, tunnel_interface: &str, allowed_traffic: &AllowedTunnelTraffic, ) -> Result<Vec<pfctl::FilterRule>> { self.get_allow_tunnel_rules_inner(tunnel_interface, allowed_traffic, pfctl::TcpFlags::new( &[pfctl::TcpFlag::Syn, pfctl::TcpFlag::Ack], &[pfctl::TcpFlag::Syn, pfctl::TcpFlag::Ack], ) ) } fn get_allow_tunnel_rules_inner( &self, tunnel_interface: &str, allowed_traffic: &AllowedTunnelTraffic, get_tcp_flags: pfctl::TcpFlags, ) -> Result<Vec<pfctl::FilterRule>> { .. }
Done.
talpid-core/src/firewall/macos.rs line 768 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
A note about the safety would be nice 😊
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 77 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Return a
Result<(ReadHalf, WriteHalf), Error>instead of callingunwrap
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 107 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Would be nice to add a comment regarding the use of
unsafe
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 117 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Would be nice to add a comment regarding the use of
unsafe
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 126 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Would be nice to add a comment regarding the use of
unsafe
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 136 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Would be nice to add a comment regarding the use of
unsafe
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 150 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Would be nice to add a comment regarding the use of
unsafe
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 156 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
You get the point
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 162 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 168 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 173 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 179 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 187 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 195 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 219 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
TODO
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 231 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
TODO
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 249 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
TODO
Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 301 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Is it obvious for the caller that
parse_bpf_headerreturnsNoneifdata.len() < size_of(bpf_hdr)? I think it deserves a mention in a doc-comment for this function 😊
Done. I've also refactored the handling of BPF payloads.
talpid-core/src/split_tunnel/macos/bpf.rs line 302 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/generate-bindings.sh line 13 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Is it a good idea to issue curl to grab the header files? Wouldn't it be nice if we could sign them? 😊
The script needs to be run manually. Normally, you'd just use the checked out files, so this should not be a problem.
talpid-core/src/split_tunnel/macos/process.rs line 134 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Is there a particular reason why
exclude_pathsis not aHashSet? 😊
Done.
talpid-core/src/split_tunnel/macos/process.rs line 148 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
ppidis a bit of an obscure name forparent_pidimho 😊
Done.
talpid-core/src/split_tunnel/macos/process.rs line 252 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Do we return early here because process corresponding to
pidis already excluded? Would be nice with a comment 😊
Done.
talpid-core/src/split_tunnel/macos/process.rs line 273 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Unsafe with safety argument :(
Done.
talpid-core/src/split_tunnel/macos/process.rs line 280 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Unsafe with safety argument :(
Done.
talpid-core/src/split_tunnel/macos/process.rs line 309 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Unsafe without safety argument :(
Done.
talpid-core/src/split_tunnel/macos/process.rs line 319 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Unsafe without safety argument :(
Done.
talpid-core/src/split_tunnel/macos/process.rs line 331 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Dead code
Done.
talpid-core/src/tunnel_state_machine/mod.rs line 510 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Promote this to a doc-comment
Done.
talpid-routing/src/lib.rs line 40 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Missing
#[cfg(target_os = "macos")]-cargo checkfails on Linux 😢
Done.
talpid-routing/src/unix/macos/interface.rs line 91 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Would be nice with a small comment explaining the use of port
0😊// Use an available random port let router_addr = SocketAddr::from((router.router_ip, 0));
The port is actually ignored here.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r8, all commit messages.
Reviewable status: 18 of 38 files reviewed, 49 unresolved discussions (waiting on @MarkusPettersson98)
talpid-core/src/split_tunnel/macos/include/bindings.h line 9 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Comment
Done.
talpid-core/src/tunnel_state_machine/disconnected_state.rs line 222 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Dito
Done.
talpid-core/src/tunnel_state_machine/disconnecting_state.rs line 83 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Dito
Done.
talpid-core/src/tunnel_state_machine/disconnecting_state.rs line 132 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Dito
Done.
talpid-core/src/tunnel_state_machine/disconnecting_state.rs line 182 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Dito
Done.
talpid-core/src/tunnel_state_machine/error_state.rs line 219 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Dito
Done.
dlon
force-pushed
the
macos-add-split-tunnel
branch
2 times, most recently
from
5fc3686 to
c37bc7e
Compare
There was a problem hiding this comment.
Reviewed 2 of 5 files at r4, 1 of 6 files at r5, 1 of 8 files at r6, 1 of 1 files at r7, 2 of 12 files at r9.
Reviewable status: 18 of 41 files reviewed, 9 unresolved discussions (waiting on @dlon)
talpid-core/src/split_tunnel/macos/bpf.rs line 301 at r2 (raw file):
Previously, dlon (David Lönnhager) wrote…
Done. I've also refactored the handling of BPF payloads.
🔥
talpid-core/src/split_tunnel/macos/bpf.rs line 315 at r5 (raw file):
} let hdr = unsafe { &*(self.data.as_ptr() as *const u8 as *const bpf_hdr) };
Unsafe without safety reasoning
Code quote:
let hdr = unsafe { &*(self.data.as_ptr() as *const u8 as *const bpf_hdr) };talpid-core/src/split_tunnel/macos/process.rs line 175 at r2 (raw file):
Previously, dlon (David Lönnhager) wrote…
Yep. I'll switch to using sets.
🔥
talpid-core/src/split_tunnel/macos/process.rs line 273 at r11 (raw file):
let buf_sz = (num_pids as usize * std::mem::size_of::<u32>()) as i32; // SAFETY: 'pids' is large enough to contain 'num_pids' processes let num_pids = unsafe { proc_listallpids(pids.as_mut_ptr() as *mut c_void, buf_sz) };
This might be too nitpicky, but it feels wrong to perform a cast from u32 to i32, which may silently overflow the result, and pass that to unsafe code. It should be fine, as num_pids should not be large enough for this to be a problem. But I also feel like simply checking the conversion at runtime and propagating the error is a low enough cost to say "just do it" 😊
Code quote:
let buf_sz = (num_pids as usize * std::mem::size_of::<u32>()) as i32;
// SAFETY: 'pids' is large enough to contain 'num_pids' processes
let num_pids = unsafe { proc_listallpids(pids.as_mut_ptr() as *mut c_void, buf_sz) };talpid-core/src/split_tunnel/macos/tun.rs line 164 at r5 (raw file):
vpn_interface: Option<VpnInterface>, classify: impl Fn(&PktapPacket) -> RoutingDecision + Send + 'static, ) -> Result<SplitTunnelHandle, Error> {
Clean API+docs! 💜
Code quote:
/// Create split tunnel device and handle all packets using `classify`. Handle any changes to the
/// default interface or gateway.
///
/// # Note
///
/// `classify` receives an Ethernet frame. The Ethernet header is not valid at this point, however.
/// Only the IP header and payload are.
pub async fn create_split_tunnel(
default_interface: DefaultInterface,
vpn_interface: Option<VpnInterface>,
classify: impl Fn(&PktapPacket) -> RoutingDecision + Send + 'static,
) -> Result<SplitTunnelHandle, Error> {talpid-core/src/split_tunnel/macos/tun.rs line 172 at r5 (raw file):
// Add IPv6 address // TODO: Only add IPv6 address if there's either a tun or a non-tun IPv6 route
TODO
Code quote:
// TODO: Only add IPv6 address if there's either a tun or a non-tun IPv6 routetalpid-core/src/split_tunnel/macos/tun.rs line 214 at r5 (raw file):
default_interface: DefaultInterface, vpn_interface: Option<VpnInterface>, }
I get what the point of this data type is, but it is a bit hard to parse as is 😅 Think of maybe adding some type aliases and/or doc comments!
Code quote:
struct RedirectHandle {
abort_tx: broadcast::Sender<()>,
ingress_task: tokio::task::JoinHandle<(
tokio::io::ReadHalf<tun::AsyncDevice>,
tokio::io::WriteHalf<tun::AsyncDevice>,
)>,
classify_task: tokio::task::JoinHandle<
Result<
(
PktapStream,
Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send + 'static>,
),
Error,
>,
>,
default_interface: DefaultInterface,
vpn_interface: Option<VpnInterface>,
}talpid-core/src/split_tunnel/macos/tun.rs line 228 at r5 (raw file):
), Error, > {
I'm a bit surprised that clippy doesn' t complain about complex types 😅
Try to make it obvious what is being returned from this function 😄
Code quote:
) -> Result<
(
tun::AsyncDevice,
PktapStream,
Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send + 'static>,
DefaultInterface,
Option<VpnInterface>,
),
Error,
> {talpid-core/src/tunnel_state_machine/mod.rs line 515 at r2 (raw file):
Previously, dlon (David Lönnhager) wrote…
The
maybe_prefix is because it only starts the machinery if
If .. ? 😄
talpid-core/src/tunnel_state_machine/mod.rs line 539 at r11 (raw file):
"{}", error.display_chain_with_msg("Failed to set VPN interface for split tunnel") );
Nit: The logging could be performed inside of inspect_err, and the mapping from error to ErrorStateCause could be contained within map_err 😊
self.runtime
.block_on(self.split_tunnel.set_tunnel(Some(vpn_interface)))
.inspect_err(|error| {
log::error!(
"{}",
error.display_chain_with_msg("Failed to set VPN interface for split tunnel")
);
})
.map_err(|error| {
if error.is_offline() {
ErrorStateCause::IsOffline
} else {
ErrorStateCause::SplitTunnelError
}
})Code quote:
log::error!(
"{}",
error.display_chain_with_msg("Failed to set VPN interface for split tunnel")
);talpid-routing/src/unix/macos/interface.rs line 91 at r2 (raw file):
Previously, dlon (David Lönnhager) wrote…
The port is actually ignored here.
Could that be the comment instead, then? 😊
There was a problem hiding this comment.
Reviewed 3 of 12 files at r9, 1 of 3 files at r10.
Reviewable status: 22 of 41 files reviewed, 9 unresolved discussions (waiting on @dlon)
talpid-routing/src/unix/macos/interface.rs line 269 at r11 (raw file):
log::debug!("Missing IP for primary interface \"{name}\""); None })?;
I like the chaining of and_then! But perhaps there are similarities between get_primary_interface and network_services that could be shared? The filter_map in network_services is out of this world 😅
Code quote:
let first_ip = global_dict
.find(ip_key)
.map(|s| unsafe { CFType::wrap_under_get_rule(*s) })
.and_then(|s| s.downcast::<CFArray>())
.and_then(|ips| {
ips.get(0)
.map(|ip| unsafe { CFType::wrap_under_get_rule(*ip) })
})
.and_then(|s| s.downcast::<CFString>())
.and_then(|ip| ip.to_string().parse().ok())
.or_else(|| {
log::debug!("Missing IP for primary interface \"{name}\"");
None
})?;
There was a problem hiding this comment.
Reviewed 1 of 5 files at r3, 4 of 12 files at r9, 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: 30 of 41 files reviewed, 11 unresolved discussions (waiting on @dlon)
mullvad-daemon/src/lib.rs line 1912 at r11 (raw file):
} else { vec![] };
May I suggest
let tunnel_list: Vec<OsString> = match update {
ExcludedPathsUpdate::SetPaths(ref paths) if settings.split_tunnel.enable_exclusions => {
paths.iter().map(OsString::from).collect()
},
ExcludedPathsUpdate::SetState(true) => {
settings.split_tunnel.apps.iter().map(OsString::from).collect()
},
_ => vec![],
};Avoids introducing intermediary variables and keeps the relevant iterators closer to the match-arms that they belong to 😊
Code quote:
let new_list = match update {
ExcludedPathsUpdate::SetPaths(ref paths) => paths.iter(),
ExcludedPathsUpdate::SetState(_) => settings.split_tunnel.apps.iter(),
};
let new_state = match update {
ExcludedPathsUpdate::SetPaths(_) => settings.split_tunnel.enable_exclusions,
ExcludedPathsUpdate::SetState(state) => state,
};
let tunnel_list = if new_state {
new_list.map(OsString::from).collect()
} else {
vec![]
};talpid-routing/src/unix/mod.rs line 107 at r11 (raw file):
oneshot::Sender<Result<Option<Route>, PlatformError>>, ), }
Would it make sense to declare this enum separately for macOS and Linux? 😊
/// Commands for the underlying route manager object.
#[cfg(target_os = "linux")]
#[derive(Debug)]
pub(crate) enum RouteManagerCommand {
AddRoutes(
HashSet<RequiredRoute>,
oneshot::Sender<Result<(), PlatformError>>,
),
ClearRoutes,
Shutdown(oneshot::Sender<()>),
CreateRoutingRules(bool, oneshot::Sender<Result<(), PlatformError>>),
ClearRoutingRules(oneshot::Sender<Result<(), PlatformError>>),
NewChangeListener(oneshot::Sender<mpsc::UnboundedReceiver<CallbackMessage>>),
GetMtuForRoute(IpAddr, oneshot::Sender<Result<u16, PlatformError>>),
/// Attempt to fetch a route for the given destination with an optional firewall mark.
GetDestinationRoute(
IpAddr,
Option<Fwmark>,
oneshot::Sender<Result<Option<Route>, PlatformError>>,
),
}
/// Commands for the underlying route manager object.
#[cfg(target_os = "macos")]
#[derive(Debug)]
pub(crate) enum RouteManagerCommand {
AddRoutes(
HashSet<RequiredRoute>,
oneshot::Sender<Result<(), PlatformError>>,
),
ClearRoutes,
Shutdown(oneshot::Sender<()>),
RefreshRoutes,
NewDefaultRouteListener(oneshot::Sender<mpsc::UnboundedReceiver<DefaultRouteEvent>>),
GetDefaultRoutes(oneshot::Sender<(Option<DefaultRoute>, Option<DefaultRoute>)>),
/// Return gateway for V4 and V6
GetDefaultGateway(oneshot::Sender<(Option<Gateway>, Option<Gateway>)>),
}Code quote:
/// Commands for the underlying route manager object.
#[derive(Debug)]
pub(crate) enum RouteManagerCommand {
AddRoutes(
HashSet<RequiredRoute>,
oneshot::Sender<Result<(), PlatformError>>,
),
ClearRoutes,
Shutdown(oneshot::Sender<()>),
#[cfg(target_os = "macos")]
RefreshRoutes,
#[cfg(target_os = "macos")]
NewDefaultRouteListener(oneshot::Sender<mpsc::UnboundedReceiver<DefaultRouteEvent>>),
#[cfg(target_os = "macos")]
GetDefaultRoutes(oneshot::Sender<(Option<DefaultRoute>, Option<DefaultRoute>)>),
/// Return gateway for V4 and V6
#[cfg(target_os = "macos")]
GetDefaultGateway(oneshot::Sender<(Option<Gateway>, Option<Gateway>)>),
#[cfg(target_os = "linux")]
CreateRoutingRules(bool, oneshot::Sender<Result<(), PlatformError>>),
#[cfg(target_os = "linux")]
ClearRoutingRules(oneshot::Sender<Result<(), PlatformError>>),
#[cfg(target_os = "linux")]
NewChangeListener(oneshot::Sender<mpsc::UnboundedReceiver<CallbackMessage>>),
#[cfg(target_os = "linux")]
GetMtuForRoute(IpAddr, oneshot::Sender<Result<u16, PlatformError>>),
/// Attempt to fetch a route for the given destination with an optional firewall mark.
#[cfg(target_os = "linux")]
GetDestinationRoute(
IpAddr,
Option<Fwmark>,
oneshot::Sender<Result<Option<Route>, PlatformError>>,
),
}
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
65ca819 to
cf634cd
Compare
There was a problem hiding this comment.
Reviewable status: 28 of 41 files reviewed, 18 unresolved discussions (waiting on @dlon)
talpid-core/src/split_tunnel/macos/tun.rs line 115 at r11 (raw file):
redir_handle: Option<RedirectHandle>, tun_name: String, }
Does redir_handle really need to be an Option<RedirectHandle>? Changing the type to simply RedirectHandle seems straightforward from my own investigations 😊
Code quote:
pub struct SplitTunnelHandle {
redir_handle: Option<RedirectHandle>,
tun_name: String,
}talpid-core/src/split_tunnel/macos/tun.rs line 279 at r11 (raw file):
vpn_interface: Option<VpnInterface>, mut classify: Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send + 'static>, ) -> Result<RedirectHandle, Error> {
This function is pretty dense .. If the code can't be made more descriptive than it already is, please add some documentation explaining the scaffolding that is done and what the spawned tasks do
Code quote:
async fn redirect_packets_for_pktap_stream(
tun_device: tun::AsyncDevice,
mut pktap_stream: PktapStream,
default_interface: DefaultInterface,
vpn_interface: Option<VpnInterface>,
mut classify: Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send + 'static>,
) -> Result<RedirectHandle, Error> {talpid-core/src/split_tunnel/macos/tun.rs line 542 at r11 (raw file):
// Drop attempt to send packets to tun IP on the real interface return; }
Silently dropping this packet might lead to headache in the future (?). Would it make sense to insert a log::trace! statement here? 😊
Code quote:
if ip.get_destination() == vpn_addr {
// Drop attempt to send packets to tun IP on the real interface
return;
}talpid-core/src/split_tunnel/macos/tun.rs line 563 at r11 (raw file):
// Drop attempt to send packets to tun IP on the real interface return; }
Silently dropping this packet might lead to headache in the future (?). Would it make sense to insert a log::trace! statement here? 😊
Code quote:
if ip.get_destination() == vpn_addr {
// Drop attempt to send packets to tun IP on the real interface
return;
}talpid-core/src/split_tunnel/macos/tun.rs line 688 at r11 (raw file):
Err(error) => Some(Err(Error::GetNextPacket(error))), } }))
Nit: This is equivalent to
Ok(stream.filter_map(|pkt| async { pkt.map_err(Error::GetNextPacket).transpose() }))With most of the noise out of the way, I think the end of this function could be re-written into this nice piece of code:
let stream = cap
.stream(PktapCodec::new(utun_iface.to_owned()))
.map_err(Error::CreateStream)?
.filter_map(|pkt| async { pkt.map_err(Error::GetNextPacket).transpose() });
Ok(stream)Code quote:
Ok(stream.filter_map(|pkt| async {
match pkt {
Ok(Some(pkt)) => Some(Ok(pkt)),
Ok(None) => None,
Err(error) => Some(Err(Error::GetNextPacket(error))),
}
}))talpid-core/src/split_tunnel/macos/tun.rs line 735 at r11 (raw file):
let mut frame = MutableEthernetPacket::owned(vec![0u8; 14 + data.len() - 4]).unwrap(); let raw_family = i32::from_ne_bytes(data[0..4].try_into().unwrap());
I think logging + propagating Noneis more robust than using unwrap in this case
// TODO: Wasteful. Could share single buffer if handling one frame at a time (assuming no
// concurrency is needed). Allocating the frame here is purely done for efficiency reasons.
let ethertype = data[0..4]
.try_into()
.inspect_err(|error| log::error!(".."))
.ok()
.map(i32::from_ne_bytes)
.and_then(|family| match family {
AF_INET => Some(EtherTypes::Ipv4),
AF_INET6 => Some(EtherTypes::Ipv6),
_ => None,
})?;
let mut frame = MutableEthernetPacket::owned(vec![0u8; 14 + data.len() - 4])?;
frame.set_ethertype(ethertype);
frame.set_payload(&data[4..]);Code quote:
let mut frame = MutableEthernetPacket::owned(vec![0u8; 14 + data.len() - 4]).unwrap();
let raw_family = i32::from_ne_bytes(data[0..4].try_into().unwrap());talpid-core/src/split_tunnel/macos/tun.rs line 760 at r11 (raw file):
let mut errbuf = [0u8; PCAP_ERRBUF_SIZE as usize]; let pcap = unsafe { pcap_create(b"pktap\0".as_ptr() as *const _, errbuf.as_mut_ptr() as _) };
C-string literals! 🎉
c"pktap"Code quote:
b"pktap\0"
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
cf634cd to
ab25222
Compare
There was a problem hiding this comment.
Reviewed 1 of 5 files at r3, 3 of 12 files at r9, 1 of 3 files at r10, 1 of 2 files at r12.
Reviewable status: 21 of 41 files reviewed, 18 unresolved discussions (waiting on @MarkusPettersson98)
mullvad-daemon/src/lib.rs line 1912 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
May I suggest
let tunnel_list: Vec<OsString> = match update { ExcludedPathsUpdate::SetPaths(ref paths) if settings.split_tunnel.enable_exclusions => { paths.iter().map(OsString::from).collect() }, ExcludedPathsUpdate::SetState(true) => { settings.split_tunnel.apps.iter().map(OsString::from).collect() }, _ => vec![], };Avoids introducing intermediary variables and keeps the relevant iterators closer to the match-arms that they belong to 😊
Nice suggestion! Done.
talpid-core/src/split_tunnel/macos/bpf.rs line 315 at r5 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Unsafe without safety reasoning
Done.
talpid-core/src/split_tunnel/macos/tun.rs line 172 at r5 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
TODO
Done.
talpid-core/src/split_tunnel/macos/tun.rs line 214 at r5 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
I get what the point of this data type is, but it is a bit hard to parse as is 😅 Think of maybe adding some type aliases and/or doc comments!
That wasn't pretty. 😅 I managed to make it simpler and added some docs.
talpid-core/src/split_tunnel/macos/tun.rs line 228 at r5 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
I'm a bit surprised that
clippydoesn' t complain about complex types 😅
Try to make it obvious what is being returned from this function 😄
I split this function into two instead. Do you find it more readable?
talpid-core/src/split_tunnel/macos/tun.rs line 115 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Does
redir_handlereally need to be anOption<RedirectHandle>? Changing the type to simplyRedirectHandleseems straightforward from my own investigations 😊
You're right. Nice catch!
talpid-core/src/tunnel_state_machine/mod.rs line 515 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
If .. ? 😄
It's gone 😄
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
954aad8 to
8728b2f
Compare
There was a problem hiding this comment.
Reviewed 9 of 13 files at r13.
Reviewable status: 19 of 41 files reviewed, 15 unresolved discussions (waiting on @MarkusPettersson98)
talpid-core/src/split_tunnel/macos/process.rs line 273 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
This might be too nitpicky, but it feels wrong to perform a cast from
u32toi32, which may silently overflow the result, and pass that to unsafe code. It should be fine, asnum_pidsshould not be large enough for this to be a problem. But I also feel like simply checking the conversion at runtime and propagating the error is a low enough cost to say "just do it" 😊
Done.
talpid-core/src/split_tunnel/macos/tun.rs line 279 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
This function is pretty dense .. If the code can't be made more descriptive than it already is, please add some documentation explaining the scaffolding that is done and what the spawned tasks do
Done.
talpid-core/src/split_tunnel/macos/tun.rs line 542 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Silently dropping this packet might lead to headache in the future (?). Would it make sense to insert a
log::trace!statement here? 😊
Done.
talpid-core/src/split_tunnel/macos/tun.rs line 563 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Silently dropping this packet might lead to headache in the future (?). Would it make sense to insert a
log::trace!statement here? 😊
Done.
talpid-core/src/split_tunnel/macos/tun.rs line 688 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Nit: This is equivalent to
Ok(stream.filter_map(|pkt| async { pkt.map_err(Error::GetNextPacket).transpose() }))With most of the noise out of the way, I think the end of this function could be re-written into this nice piece of code:
let stream = cap .stream(PktapCodec::new(utun_iface.to_owned())) .map_err(Error::CreateStream)? .filter_map(|pkt| async { pkt.map_err(Error::GetNextPacket).transpose() }); Ok(stream)
Nice! Fixed.
talpid-core/src/split_tunnel/macos/tun.rs line 735 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
I think logging + propagating
Noneis more robust than usingunwrapin this case// TODO: Wasteful. Could share single buffer if handling one frame at a time (assuming no // concurrency is needed). Allocating the frame here is purely done for efficiency reasons. let ethertype = data[0..4] .try_into() .inspect_err(|error| log::error!("..")) .ok() .map(i32::from_ne_bytes) .and_then(|family| match family { AF_INET => Some(EtherTypes::Ipv4), AF_INET6 => Some(EtherTypes::Ipv6), _ => None, })?; let mut frame = MutableEthernetPacket::owned(vec![0u8; 14 + data.len() - 4])?; frame.set_ethertype(ethertype); frame.set_payload(&data[4..]);
Panicking is probably preferable here. (Discussed offline.)
talpid-core/src/split_tunnel/macos/tun.rs line 760 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
C-string literals! 🎉
c"pktap"
Done.
talpid-routing/src/unix/macos/interface.rs line 269 at r11 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
I like the chaining of
and_then! But perhaps there are similarities betweenget_primary_interfaceandnetwork_servicesthat could be shared? Thefilter_mapinnetwork_servicesis out of this world 😅
Done.
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
8728b2f to
6b15e64
Compare
There was a problem hiding this comment.
Reviewed 2 of 13 files at r13, 1 of 5 files at r14, 3 of 3 files at r16, 1 of 1 files at r17.
Reviewable status: 23 of 41 files reviewed, 6 unresolved discussions (waiting on @dlon)
talpid-core/src/firewall/mod.rs line 128 at r2 (raw file):
/// Interface to redirect (VPN tunnel) traffic to #[cfg(target_os = "macos")] redirect_interface: Option<String>,
Nit Would be nice with a more descriptive type than Option<String>.. 😄
Code quote:
/// Interface to redirect (VPN tunnel) traffic to
#[cfg(target_os = "macos")]
redirect_interface: Option<String>,talpid-core/src/firewall/mod.rs line 144 at r2 (raw file):
/// Interface to redirect (VPN tunnel) traffic to #[cfg(target_os = "macos")] redirect_interface: Option<String>,
Nit Would be nice with a more descriptive type than Option<String>.. 😄
Code quote:
/// Interface to redirect (VPN tunnel) traffic to
#[cfg(target_os = "macos")]
redirect_interface: Option<String>,talpid-core/src/split_tunnel/macos/process.rs line 120 at r17 (raw file):
} } Err(Error::MonitorFailed(io::Error::new(io::ErrorKind::Other, format!("eslogger stopped unexpectedly: {status}"))))
Tip Can use std::io::Error::other instead of constructing this particular IO error manually 😊
Code quote:
Err(Error::MonitorFailed(io::Error::new(io::ErrorKind::Other, format!("eslogger stopped unexpectedly: {status}"))))talpid-core/src/split_tunnel/macos/tun.rs line 214 at r5 (raw file):
Previously, dlon (David Lönnhager) wrote…
That wasn't pretty. 😅 I managed to make it simpler and added some docs.
🙌
talpid-core/src/split_tunnel/macos/tun.rs line 228 at r5 (raw file):
Previously, dlon (David Lönnhager) wrote…
I split this function into two instead. Do you find it more readable?
Yes, definitely! Nice job! 😄
talpid-core/src/split_tunnel/macos/tun.rs line 172 at r17 (raw file):
io::ErrorKind::Other, "ifconfig failed", )));
Tip use io::Error::other instead of manuall specifying the type
return Err(Error::AddIpv6Address(io::Error::other(
"ifconfig failed",
)));Code quote:
return Err(Error::AddIpv6Address(io::Error::new(
io::ErrorKind::Other,
"ifconfig failed",
)));talpid-core/src/split_tunnel/macos/tun.rs line 264 at r17 (raw file):
/// Monitor outgoing traffic on `st_tun_device` using `pktap_stream`. A routing decision is made for /// each packet using `classify`. Based on this, a packet is forced out on either /// `default_interface` or `vpn_interface`.
Based on this, a packet is either dropped or forced out on either default_interface or vpn_interface.
Code quote:
/// each packet using `classify`. Based on this, a packet is forced out on either
/// `default_interface` or `vpn_interface`.talpid-core/src/split_tunnel/macos/tun.rs line 295 at r17 (raw file):
bpf::BpfStream::from_read_half(default_read).map_err(Error::CreateDefaultBpf)?; let (abort_tx, abort_rx) = broadcast::channel(5);
I think the capacity could safely be reduced to 1 😊
Code quote:
let (abort_tx, abort_rx) = broadcast::channel(5);talpid-core/src/split_tunnel/macos/tun.rs line 772 at r17 (raw file):
frame.set_ethertype(ethertype); frame.set_payload(&data[4..]);
Tip Perfect use case for slice::split_first_chunk!
let (raw_family, payload) = data.split_first_chunk::<4>()?;
let ethertype = match i32::from_ne_bytes(*raw_family) {
AF_INET => EtherTypes::Ipv4,
AF_INET6 => EtherTypes::Ipv6,
_ => return None,
};
frame.set_ethertype(ethertype);
frame.set_payload(payload);Code quote:
let raw_family = i32::from_ne_bytes(data[0..4].try_into().unwrap());
let ethertype = match raw_family {
AF_INET => EtherTypes::Ipv4,
AF_INET6 => EtherTypes::Ipv6,
_ => return None,
};
frame.set_ethertype(ethertype);
frame.set_payload(&data[4..]);
dlon
force-pushed
the
fix-flaky-lan-lockdown-test
branch
2 times, most recently
from
b30d838 to
84d15c4
Compare
dlon
force-pushed
the
macos-add-split-tunnel
branch
5 times, most recently
from
024baea to
7dcf3cd
Compare
There was a problem hiding this comment.
Reviewed 3 of 14 files at r25, 6 of 10 files at r26, 8 of 11 files at r27, 12 of 12 files at r28, all commit messages.
Reviewable status: 44 of 53 files reviewed, 3 unresolved discussions (waiting on @dlon)
talpid-core/src/split_tunnel/macos/bpf.rs line 312 at r2 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Nit A bit esoteric for people not familiar with bpf - would it be okay to add a doc-comment? If it is overkill, skip it 😊
Sorry for resurrecting this already-resolved thread, but I now realize that the computation itself is not explained, which is a bummer. It's great that there exist a simple unit test to demonstrate the expected behavior for certain values of n, but it still takes a moment to work out what is actually going on.. It would be nice to add a small comment (ascii-art? 😄) explaining a "word" and it's "next word boundary". For example, one things that is hard to work out by just looking at the code + function type signature is that the input and output is implicitly a number of bytes.
talpid-core/src/split_tunnel/macos/mod.rs line 162 at r26 (raw file):
self.state.fail(); }
🧑🍳 💋
Code quote:
if self.state.active() {
if let Some(tunnel_tx) = self.tunnel_tx.upgrade() {
let _ = tunnel_tx.unbounded_send(TunnelCommand::Block(ErrorStateCause::SplitTunnelError));
}
}
self.state.fail();
}talpid-core/src/split_tunnel/macos/tun.rs line 368 at r28 (raw file):
pktap_stream: PktapStream, classify: Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send>, }
Nit: I feel like classify maybe deserves its own type alias, since it is used in quite a few places. Also, it seems to be used where PktapStream shows up, so grouping these two type aliases can serve as some sort of documentation of this module 😊
type PktapStream = std::pin::Pin<Box<dyn Stream<Item = Result<PktapPacket, Error>> + Send>>;
+/// TODO: Document
+/// TODO: Rename
+type Thunk = Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send + 'static>;This will require some refactoring of existing code, but it just boils down to when the classify function is boxed 😊
Code quote:
struct EgressResult {
pktap_stream: PktapStream,
classify: Box<dyn Fn(&PktapPacket) -> RoutingDecision + Send>,
}talpid-routing/src/unix/macos/interface.rs line 178 at r28 (raw file):
); error })
Nit Prefer inspect_err if the intention is to only log the error 😊
let index = if_nametoindex(iface.name.as_str())
.inspect_err(|error| {
log::error!(
"Failed to retrieve interface index for \"{}\": {error}",
iface.name
);
})
.ok()?;Code quote:
.map_err(|error| {
log::error!(
"Failed to retrieve interface index for \"{}\": {error}",
iface.name
);
error
})
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
4b5bf69 to
2577732
Compare
There was a problem hiding this comment.
Reviewed 6 of 6 files at r29, all commit messages.
Reviewable status: 44 of 53 files reviewed, all discussions resolved (waiting on @dlon)
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
25f15ec to
441b25e
Compare
dlon
force-pushed
the
macos-add-split-tunnel
branch
from
441b25e to
818ca7a
Compare
There was a problem hiding this comment.
Reviewed 3 of 14 files at r25, 1 of 1 files at r30, all commit messages.
Reviewable status: 47 of 53 files reviewed, all discussions resolved (waiting on @dlon)
See the title.
Fixes DES-576.
Fixes DES-701.
Fixes DES-648.
This change is