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

Fix bug when pulling Windows net interfaces #330

Merged
merged 2 commits into from May 29, 2018

Conversation

Projects
None yet
2 participants
@JuxhinDB
Contributor

JuxhinDB commented May 27, 2018

The following PR is related to #329


Stdout
extern crate pnet_datalink;

fn send_tcp_packet() {
    let foo = pnet_datalink::interfaces();
    print!("{:?}", foo);
}


fn main() {
    send_tcp_packet();
}
$ cargo build
   Compiling pnet_datalink v0.21.0 (file:///C:/Users/JDB/Projects/libpnet/pnet_datalink)
   Compiling pnet v0.21.0 (file:///C:/Users/JDB/Projects/libpnet)
    Finished dev [unoptimized + debuginfo] target(s) in 1.71 secs
$ cargo run
   Compiling pnet_datalink v0.21.0 (file:///C:/Users/JDB/Projects/libpnet/pnet_datalink)
   Compiling pnet v0.21.0 (file:///C:/Users/JDB/Projects/libpnet)
    Finished dev [unoptimized + debuginfo] target(s) in 1.72 secs
     Running `target\debug\pnet.exe`
[NetworkInterface { name: "\\Device\\NPF_{AD9EB8E3-2ED9-4AA0-A2D6-FB1228C69FA0}", index: 20, mac: Some(00:1a:7d:da:71:02), ips: [V4(Ipv4Network { addr: 0.0.0.0, prefix: 0 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{F34B0E4A-8171-49E4-BBAD-FCEB0B58E934}", index: 25, mac: Some(00:15:5d:00:02:1a), ips: [V4(Ipv4Network { addr: 10.0.75.1, prefix: 24 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{686A554C-5292-4071-8A1B-B6031C857CD7}", index: 12, mac: Some(70:85:c2:0b:7f:dc), ips: [V4(Ipv4Network { addr: 192.168.1.102, prefix: 24 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{7101BD7D-4CDF-4845-810F-D60F043DA4E2}", index: 13, mac: Some(00:15:5d:8a:d4:25), ips: [V4(Ipv4Network { addr: 172.19.80.1, prefix: 20 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{D9045567-E728-4DB1-96C4-18971241D5F6}", index: 23, mac: Some(16:15:7d:77:ff:87), ips: [V4(Ipv4Network { addr: 172.30.107.81, prefix: 28 })], flags: 0 }]

Description

When calling winpcap::PacketGetAdapterNames the initial buffer length was being set to 4096, which may not always be sufficient (i.e. Windows with multiple Hyper-V adapters), however when the call fails, the buflen is mutated to the correct value.

In that case, buf is adjusted to me 4096 bytes + buflen which should always align correctly. Finally, a second call to winpcap::PacketGetAdapterNames is made with the new correct buffer size.


PS: I'm not too proficient in Rust unfortunately, so the code may not turn out too nice. I tried wrapping the winpcap function call in a closure, however it kept on causing all sorts of reference issues that I could not solve. :-)

Fixed bug when pulling Windows net interfaces
- Related to #329
- If initial buffer size is not sufficient, the buffer is aligned with the new correct buffer length and the winpcap call is repeated

@JuxhinDB JuxhinDB changed the title from Fixed bug when pulling Windows net interfaces to (WIP) Fix bug when pulling Windows net interfaces May 27, 2018

@JuxhinDB

This comment has been minimized.

Contributor

JuxhinDB commented May 27, 2018

EDIT

Turns out you need to run command prompt as Administrator to get the list of Network Adapters!!

C:\Users\JDB\Projects\libpnet>cargo run
   Compiling pnet_datalink v0.21.0 (file:///C:/Users/JDB/Projects/libpnet/pnet_datalink)
   Compiling pnet v0.21.0 (file:///C:/Users/JDB/Projects/libpnet)
    Finished dev [unoptimized + debuginfo] target(s) in 1.72 secs
     Running `target\debug\pnet.exe`
[NetworkInterface { name: "\\Device\\NPF_{AD9EB8E3-2ED9-4AA0-A2D6-FB1228C69FA0}", index: 20, mac: Some(00:1a:7d:da:71:02), ips: [V4(Ipv4Network { addr: 0.0.0.0, prefix: 0 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{F34B0E4A-8171-49E4-BBAD-FCEB0B58E934}", index: 25, mac: Some(00:15:5d:00:02:1a), ips: [V4(Ipv4Network { addr: 10.0.75.1, prefix: 24 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{686A554C-5292-4071-8A1B-B6031C857CD7}", index: 12, mac: Some(70:85:c2:0b:7f:dc), ips: [V4(Ipv4Network { addr: 192.168.1.102, prefix: 24 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{7101BD7D-4CDF-4845-810F-D60F043DA4E2}", index: 13, mac: Some(00:15:5d:8a:d4:25), ips: [V4(Ipv4Network { addr: 172.19.80.1, prefix: 20 })], flags: 0 }, NetworkInterface { name: "\\Device\\NPF_{D9045567-E728-4DB1-96C4-18971241D5F6}", index: 23, mac: Some(16:15:7d:77:ff:87), ips: [V4(Ipv4Network { addr: 172.30.107.81, prefix: 28 })], flags: 0 }]

Since winpcap does not seem to be raising any permission exception(s), should I still leave this as is?


@mrmonday -- The above PR solves the panic! issue, however there seems to be some other odd behavior I can't quite figure out. Take the following snippet:

    let mut buf = [0u8; 524288]; 
    let mut buflen = buf.len() as u32; 

    println!("Initial buffer size: {:?}", buflen);

    let outcome = unsafe {
        winpcap::PacketGetAdapterNames(buf.as_mut_ptr() as *mut i8, &mut buflen)
    };

    println!("PacketGetAdapterNames output: {:?}", outcome);
    println!("Required buffer size: {:?}", buflen);
    println!("Output buffer: {:?}", &buf[..]);

I'm creating an obnoxiously large buffer assuming that it will more than fit the network adapters, and even if it doesn't I was expecting the winpcap::PacketGetAdapterNames call to set buflen to the buffer size required anyway. Unfortunately the output does not seem to populate the buffer and not only that, buflen is getting set to 0 rather than the requested size.

Initial buffer size: 524288
PacketGetAdapterNames output: 0
Required buffer size: 0
Output buffer: [0, 0, ... , 0, 0]

Which seems to be a dead-end for me. Do you know if there is any available source of documentation for winpcap PacketGetAdapterNames?

@@ -324,17 +324,17 @@ pub fn interfaces() -> Vec<NetworkInterface> {
}
}
let mut buf = [0u8; 4096];
const IFACE_BUF_LEN: usize = 4096;
let mut buf = vec![0u8; IFACE_BUF_LEN];

This comment has been minimized.

@mrmonday

mrmonday May 28, 2018

Contributor

I wouldn't bother with the constant - it's only used here, so doesn't really add anything

// - size should be buf.len() + buflen (buflen is overwritten)
panic!("FIXME [windows] unable to get interface list");
buf = vec![0u8; IFACE_BUF_LEN + buflen as usize];

This comment has been minimized.

@mrmonday

mrmonday May 28, 2018

Contributor

The new buffer length should be buflen, not the initial length + buflen?

Also, you may want to use this method, rather than allocating a new vector: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.resize

panic!("FIXME [windows] unable to get interface list");
buf = vec![0u8; IFACE_BUF_LEN + buflen as usize];
unsafe { winpcap::PacketGetAdapterNames(buf.as_mut_ptr() as *mut i8, &mut buflen) };

This comment has been minimized.

@mrmonday

mrmonday May 28, 2018

Contributor

Please check the return value again here - panic if it returns 0.

@mrmonday

This comment has been minimized.

Contributor

mrmonday commented May 28, 2018

Thanks for working on this!

There used to be some documentation somewhere, I can't find it though - it's possibly included with the winpcap release source.

Source can be found here (might not be the exact version used): https://github.com/SoftEtherVPN/Win10Pcap/blob/57da63acb45fb9b693c7fbf766253b556cbdcef6/Packet_dll/Packet32.c#L593-L658

If you can reliably detect permissions issues, I'd love another PR for that. I say reliably, because I've tried detecting whether the running application has sufficient privileges to do things on windows before, and it has always seemed hard to get it right in all circumstances. Might not be the case here.

With your snippet - do you get the same results if you use a vector, rather than an array?

Juxhin Dyrmishi Brigjaj
@JuxhinDB

This comment has been minimized.

Contributor

JuxhinDB commented May 28, 2018

@mrmonday -- Nothing at all, it's been a pleasure. If only you knew how much time I spent trying to figure out how to re-allocate the buffer correctly, only for it to be so much simpler. :-)

The buffer will now increase in size and pad the extended elements with 0s. The reason I initially set the new buffer length to 4096 + buflen was due to an old comment on the code that was making me scratch my head:

size should be buf.len() + buflen (buflen is overwritten)

And I thought it would be better to be 4096 bytes too long rather than too short. But I can confirm that this now works correctly just by using buflen.

I added another panic! as requested, however I did not set the message to FIX ME [windows] ... because that panic! should not really ever happen and if it does, it means something else blew up.

Regarding the permission issue; I haven't had the time to look at it yet. However if you would like me to, I would be more than happy to. Or maybe help fix some other Windows related FIXME(s) for you given I have the environment setup.

@mrmonday mrmonday changed the title from (WIP) Fix bug when pulling Windows net interfaces to Fix bug when pulling Windows net interfaces May 29, 2018

@mrmonday mrmonday merged commit e817822 into libpnet:master May 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mrmonday

This comment has been minimized.

Contributor

mrmonday commented May 29, 2018

Looking great, thanks!

It would be great if you could look into some of the other FIXMEs if you have time - happy to mentor if you need a hand.

The most likely scenario for this to happen (I would imagine) is when the interface list changes in between the two calls - that's pretty unlikely though.

For little things like that, I can recommend searching the docs I linked for keywords, or asking in the Rust IRC channels if you can't find what you need. They also like pull requests if something is missing - if you struggle to find something, the chances are someone else will too :)

@JuxhinDB

This comment has been minimized.

Contributor

JuxhinDB commented May 29, 2018

Awesome, I'll check out some of the other FIXMEs towards the weekend and will open up additional PRs. Thanks for the help! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment