Add a binding for pcap_loop

[saethlin]

Resuming #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]

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]

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]

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.

[Wojtek242]

@saethlin thanks for the contribution! I have now also looked at the code (only two minor comments) and I would be happy to merge it once all the issues, including unit tests, are resolved.

The unit tests are a new addition to allow this repo to test Windows without running integration tests which cause most pipelines to fail due to an error that doesn't seem to be an issue outside of CI (#275). Since they're new, I expect some teething pains so I'm happy to help out in their prep. My suggestion would be to first look at the other tests in the module as they all have a similar predictable pattern.

I also agree with your comment about unsafe. This function itself does not need to be unsafe.

[Stargateur]

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]

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]

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.

[Wojtek242]

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.

Thanks for the ping! I wasn't sure if you're done with the changes. The changes so far look good, but you still haven't handled the return code of pcap_loop. Is that something you think you could still handle? The rest looks good to me as is.

[saethlin]

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]

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]

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]

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