search

rust-pcap / pcap

Rust language pcap library
Apache License 2.0
594 stars 138 forks source link

Add a binding for pcap_loop #335

Closed saethlin closed 3 months ago

saethlin commented 4 months ago

Resuming https://github.com/rust-pcap/pcap/pull/310

I recently ran into a situation where a tool I help maintain and tcpdump had different behavior with a somewhat exotic libpcap implementation. I eventually tracked this down to the fact that tcpdump uses pcap_loop and since said tool is going through this crate, it was using pcap_next_ex. This difference was actually a bug in the libpcap implementation, I spent a frustrating amount of time convincing people that the bug was in libpcap, because I couldn't closely imitate the libpcap calls of tcpdump.

Since that time, I've been wanting a way to call pcap_loop from this crate, because that would have made my life a lot easier. So I finally got around to putting one together, here it is.

I don't really care about the public API. Ideas like a better name are very welcome.

saethlin commented 4 months ago

On the previous PR, @Stargateur said

this method is unsafe by nature, since it's will call rust from C

I don't agree with this statement. A safe function is only unsound if a call to it can cause safe code to execute UB. Can you explain how the API I'm proposing to add can enable safe code to execute UB?

saethlin commented 4 months ago

I'm not sure what to do about the coverage regression on Windows. I feel like I'm already hacking the metrics to add the panic-path coverage on Linux. Amusingly, cargo-llvm-cov does not agree that this crate has 100% code coverage due to uncovered error paths, but I suppose grcov doesn't understand those.

Wojtek242 commented 4 months ago

Windows doesn't run integration tests due to #275. To fix coverage, please add unit tests to src/capture/activated/mod.rs as well (or instead) where the functions are tested against a mock raw module. They don't have to be strict, but it allows to validate that the right pcap functions are being called and that there's nothing extra happening that we do not expect.

If you don't have a Windows machine you can check that coverage locally by running cargo test --lib rather than running all tests.

Stargateur commented 4 months ago

On the previous PR, @Stargateur said

this method is unsafe by nature, since it's will call rust from C

I don't agree with this statement. A safe function is only unsound if a call to it can cause safe code to execute UB. Can you explain how the API I'm proposing to add can enable safe code to execute UB?

if you catch unwind in the function like you did I think it should be safe. Unfortunately I expect some cost in runtime for this.

saethlin commented 4 months ago

Unfortunately I expect some cost in runtime for this.

That's a good question. Currently I can't figure out what I'd change about this codegen though: https://godbolt.org/z/xe7xzEeTa

It's just a straight call and return on the non-panicking path. Perhaps there's some inlining impact, but it also looks like all the code falls away if we compile with aborting panics.

saethlin commented 3 months ago

ping @Wojtek242 I don't know how you manage your review queue so I'm generating a notification for you in case that's how.

saethlin commented 3 months ago

Right! I gave it a shot. It's not clear to me how errors from the library are supposed to be handled; I see pcap_loop returns an error but also the error is available from pcap_geterr? So I just followed your advice, I think.

saethlin commented 3 months ago

It looks like CI is trying to update rustup but is not allowed to on Windows:

info: checking for self-update
info: downloading self-update
error: could not remove 'rustup-bin' file: 'C:\Users\runneradmin\.cargo\bin\rustup.exe'

Caused by:
    Access is denied. (os error 5)error: could not remove 'setup' file: 'C:\Users\runneradmin\.cargo\bin/rustup-init.exe'

Caused by:
    Access is denied. (os error 5)

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: BaseThreadInitThunk
   9: RtlUserThreadStart
Wojtek242 commented 3 months ago

The changes look good to me! Yea, I've seen the rustup failures on windows on the nightly CI. I have no idea why they decided to fail now all of a sudden. Either way that won't block this PR. I'll merge this soon-ish when I'll also have some time to push an update to crates.

saethlin commented 3 months ago

They're probably failing because there was recently a rustup release. Previously, the code wouldn't try to actually update the binary. https://blog.rust-lang.org/2024/03/11/Rustup-1.27.0.html