Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added 'get_mac_address_by_ip' which does what it says #42

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions examples/lookup_by_ip.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use std::{net::IpAddr, str::FromStr};

use mac_address::get_mac_address_by_ip;

// UDP 'connect' to a remote IP (Google's DNS) and
// then see which local IP address we were bound to.
//
// NOTE: this is a nice portable way to use the routing
// table and sends no actual packets
fn lookup_default_adapter_ip() -> std::io::Result<IpAddr> {
let udp_sock = std::net::UdpSocket::bind(("0.0.0.0", 0))?;
udp_sock.connect((IpAddr::from_str("8.8.8.8").unwrap(), 53))?;
Ok(udp_sock.local_addr()?.ip())
}

fn main() -> std::io::Result<()> {
// find a useful IP local address to test against
let local_ip = lookup_default_adapter_ip()?;
// find the MacAddress of the Adapter with this IP
match get_mac_address_by_ip(&local_ip) {
Ok(Some(ma)) => {
println!("MAC addr = {}", ma);
println!("bytes = {:?}", ma.bytes());
}
Ok(None) => println!("No MAC address found."),
Err(e) => println!("{:?}", e),
}
Ok(())
}
9 changes: 9 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ mod os;
mod os;

mod iter;
use std::net::IpAddr;

pub use iter::MacAddressIterator;

/// Possible errors when attempting to retrieve a MAC address.
Expand Down Expand Up @@ -128,6 +130,13 @@ pub fn name_by_mac_address(mac: &MacAddress) -> Result<Option<String>, MacAddres
os::get_ifname(&mac.bytes)
}

/// Attempts to look up the local MAC address by matching the Apapter's IP Address
pub fn get_mac_address_by_ip(ip: &IpAddr) -> Result<Option<MacAddress>, MacAddressError> {
let bytes = os::get_mac_address_by_ip(ip)?;

Ok(bytes.map(|b| MacAddress { bytes: b }))
}

impl MacAddress {
/// Returns the array of MAC address bytes.
pub fn bytes(self) -> [u8; 6] {
Expand Down
53 changes: 53 additions & 0 deletions src/linux.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#![allow(dead_code)]

use std::{
collections::HashMap,
net::{IpAddr, Ipv4Addr, Ipv6Addr},
};

use crate::MacAddressError;
use nix::ifaddrs::*;

Expand Down Expand Up @@ -30,6 +35,54 @@ pub fn get_mac(name: Option<&str>) -> Result<Option<[u8; 6]>, MacAddressError> {
Ok(None)
}

/// Uses the `getifaddrs` call to retrieve a list of network interfaces on the
/// host device and returns the MAC address that matching the adapter with
/// the given IP
///
/// Because nix returns all of the IP's and MAC's in a combined list, we need
/// to map the IP to an inteface name and track the MAC addresses by interface name
/// and see if there's a match
pub fn get_mac_address_by_ip(ip: &IpAddr) -> Result<Option<[u8; 6]>, MacAddressError> {
let ifiter = getifaddrs()?;

let mut ip_on_inferface: Option<String> = None;
let mut mac_to_interface: HashMap<String, Option<[u8; 6]>> = HashMap::new();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused what the purpose of this HashMap is, if we already have the interface IP and MAC while we're iterating over them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, as I understand it, the getifaddrs()'s call returns all of the addresses (MAC, IPv4, IPv6, etc.) of all of the interfaces on the system in a flat structure, e.g., there's no single record that has both the MAC and the IP address. So there will (ideally) be one record with the target IP and a different record with the resulting MAC address. The HashMap is just creating an index/map from each interface to its MAC address so, for example, if we find the MAC address entry first, we record it and then if we find the IP entry later, we can look it up and return it. Similarly, because the order of the records that getifaddrs() returns isn't guaranteed, we could in theory find the IP address we're looking for first, and then the MAC address in a later record, so we have to handle that case as well.

Does that make sense? Did I understand it correctly? FYI: I did look in the nix crate for a more direct function to do the mapping but couldn't find it, so there may be a better way to do this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reviews! FYI: I ran this on both a Linux and Windows system and "it worked for me" but it's wasn't clear how to do deeper testing for this type of tightly coupled system calls. Let me know about the rest and I'll do another pass on the code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see. Thanks for the info, I'll do some further investigating and see what I can find wrt. that specific bit of code 🤔

for interface in ifiter {
if let Some(iface_address) = interface.address {
// is this a mac address?
if let Some(link) = iface_address.as_link_addr() {
mac_to_interface.insert(interface.interface_name.clone(), link.addr());
// did we just find what we're looking for?
if let Some(intf_name) = &ip_on_inferface {
if *intf_name == interface.interface_name {
return Ok(link.addr());
}
}
}
if let Some(adapter_ip) = if let Some(sin4) = iface_address.as_sockaddr_in() {
// v4 addr?
Some(IpAddr::from(Ipv4Addr::from(sin4.ip())))
} else if let Some(sin6) = iface_address.as_sockaddr_in6() {
// v6 addr?
Some(IpAddr::from(Ipv6Addr::from(sin6.ip())))
} else {
// something else, ignore
None
} {
// found an IP for this adapter - if it's the one we're looking for, save it
if adapter_ip == *ip {
ip_on_inferface = Some(interface.interface_name.clone());
if let Some(mac) = mac_to_interface.get(&interface.interface_name) {
return Ok(mac.clone());
}
}
}
Comment on lines +62 to +79
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is pretty confusing, using if let ...s for the condition of another if let directly is also pretty unidiomatic, so I think it'd be better if you bound the IP to a variable first, and continue on None instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree about it being un-idiomatic - this is fairly standard functional programming which Rust explicitly tries to support (https://doc.rust-lang.org/book/ch13-00-functional-features.html). That said, it's your code and I'd rather you agree to land it than argue about it so I can change it. Let me know if the my response to your first comment makes sense and then I can tackle any changes together with this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't call that style functional, it's an if ... else if ... else that evaluates to a bool, but regardless, my concern is it took me about ~5 minutes to figure out how that was compiling until I finally broke it down (and I work with Rust almost every day and have seen it before 😄), its just pretty hard to read IMO with the condition being at the same level as the actual following block, and } { being pretty uncommon, and would better be served by a .map or .and_then on another binding (or even as the conditional).

}
}

Ok(None)
}

pub fn get_ifname(mac: &[u8; 6]) -> Result<Option<String>, MacAddressError> {
let ifiter = getifaddrs()?;

Expand Down
74 changes: 73 additions & 1 deletion src/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,88 @@ use std::{
convert::{TryFrom, TryInto},
ffi::CStr,
ffi::OsString,
net::IpAddr,
os::windows::ffi::OsStringExt,
ptr, slice,
};
use winapi::shared::{ntdef::ULONG, winerror::ERROR_SUCCESS, ws2def::AF_UNSPEC};
use winapi::shared::{
ntdef::ULONG,
winerror::ERROR_SUCCESS,
ws2def::{AF_INET, AF_INET6, AF_UNSPEC, SOCKADDR_IN},
ws2ipdef::SOCKADDR_IN6,
};
use winapi::um::{iphlpapi::GetAdaptersAddresses, iptypes::IP_ADAPTER_ADDRESSES_LH};

use crate::MacAddressError;

const GAA_FLAG_NONE: ULONG = 0x0000;

/// Similar to get_mac(); walk this list of adapters and in each
/// adapter, walk the list of UnicastIpAddresses and return the mac address
/// of the first one that matches the given IP
pub(crate) fn get_mac_address_by_ip(ip: &IpAddr) -> Result<Option<[u8; 6]>, MacAddressError> {
let adapters = get_adapters()?;
// Safety: We don't use the pointer after `adapters` is dropped
let mut ptr = unsafe { adapters.ptr() };

// for each adapter on the machine
while !ptr.is_null() {
let bytes = unsafe { convert_mac_bytes(ptr) };

let mut ip_ptr = unsafe { (*ptr).FirstUnicastAddress };
// for each IP on the adapter
while !ip_ptr.is_null() {
let sock_addr = unsafe { (*ip_ptr).Address.lpSockaddr };
let adapter_ip = match unsafe { (*sock_addr).sa_family } as i32 {
AF_INET => unsafe {
let addr = (*(sock_addr as *mut SOCKADDR_IN)).sin_addr;
Some(IpAddr::from([
addr.S_un.S_un_b().s_b1,
addr.S_un.S_un_b().s_b2,
addr.S_un.S_un_b().s_b3,
addr.S_un.S_un_b().s_b4,
]))
},
AF_INET6 => unsafe {
let addr = (*(sock_addr as *mut SOCKADDR_IN6)).sin6_addr;
Some(IpAddr::from(addr.u.Byte().clone()))
},
_ => {
// ignore unknown address families; only support IPv4/IPv6
None
}
};
Comment on lines +37 to +55
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think continueing here and advancing the pointer would again be cleaner so that you don't need to continually check for Somes. I'll have to review the rest of the code more closely on a Windows machine as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - I agree. I can fix this up. Let me know about the above and I'll change it all at once.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should solve my concerns for the time being then with the above feedback, I'll punt on the HashMap stuff for the time being (I want to do a bigger overhaul of the entire crate here at some point anyway so I'll investigate that closer then), so just let me know whenever things are updated and I'll merge 👍

if let Some(adapter_ip) = adapter_ip {
if adapter_ip == *ip {
return Ok(Some(bytes));
}
}
// Otherwise check the next IP on the adapter
#[cfg(target_pointer_width = "32")]
{
ip_ptr = unsafe { ip_ptr.read_unaligned().Next };
}

#[cfg(not(target_pointer_width = "32"))]
{
ip_ptr = unsafe { (*ip_ptr).Next };
}
}

// Otherwise go to the next adapter
#[cfg(target_pointer_width = "32")]
{
ptr = unsafe { ptr.read_unaligned().Next };
}

#[cfg(not(target_pointer_width = "32"))]
{
ptr = unsafe { (*ptr).Next };
}
}
// All of the calls succeeded, just didn't find it...
Ok(None)
}
/// Uses bindings to the `Iphlpapi.h` Windows header to fetch the interface
/// devices list with
/// [GetAdaptersAddresses][https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx]
Expand Down
Loading