Add a binding for pcap_breakloop

[cai-william]

Intent of this PR is to provide a method to stop a blocking capture loop in a multithreaded application so it can handle graceful shutdowns.

Most of the implementation is taken from @FeldrinH's comment here

I've tested this works on Linux, however this is untested on Windows.

Fixes #312

[Stargateur]

This code could lead to future problem, this hand solution to secure handle is not very rusty. If we need to share handle more safely maybe it would be better to change handle to Arc<RwLock<Option<NonNull<raw::pcap_t>>>>

[FeldrinH]

I agree that it's not very idiomatic, but having RwLock on the entire handle would be bad for performance and require changes to lots of existing code. My little trick allows all existing code to keep working unchanged and the only place where you have to be careful about using the handle is outside of Capture.

[Stargateur]

In that case use the weak feature of Arc, do Arc<Handle> downgrade the handle to Weak<Handle> when needed and add a drop implementation on Handle that free the handle, it should work safely without mutex. That better than the current proposition in my eye.

[Wojtek242]

I agree with @Stargateur that the current solution is not idiomatic and clunky. But I also understand where you are coming from and your concerns about code churn, @FeldrinH.

This looks like a very useful functionality to have in the code base and since it already came up before, I would like to help make this PR get accepted. But first let's see if we can make it less clunky.

Our Capture is only Send and not Sync. As I see it there are two potential options:

1. Just wrap the handle in an Arc<RwLock<>> adjust the code and deal with the overhead.
2. Create a new type CaptureMt or something which basically just wraps Capture in a suitable lock and thus implements Sync. Implement some convenience methods to acquire the lock on the underlying Capture and implement breakloop functionality only on this variant.

What do you think? Option 2 is some extra work but seems fairly scoped.

[FeldrinH]

Personally I would prefer some clunkyness in the internal implementation over compromising the performance or complexity of public API.

Also, I think option 2 doesn't work in practice, because to read packets from the capture you need mutable access to the capture, which would mean an exclusive lock. The point of breakloop is to interrupt this operation, but that is not possoble because breakloop has to lock the capture first and thus would block until a packet is read.

[Wojtek242]

So I've done some reading up and I propose the following.

Summary: I think that to implement this feature idiomatically the NonNull<pcap_t> should be wrapped in Arc (but without a lock) and the BreakLoop struct should acquire a Weak downgrade of Arc as suggested by @Stargateur.

This will work because pcap_breakloop is thread-safe and can be called on a valid pcap_t pointer without any additional locking. From https://www.tcpdump.org/manpages/pcap_breakloop.3pcap.html:

This routine is safe to use inside a signal handler on UNIX or a console control handler on Windows, or in a thread other than the one in which the loop is running, as it merely sets a flag that is checked within the loop and, on some platforms, performs a signal-safe and thread-safe API call.

So the only thing that needs to be ensured is that the pointer points to valid data and has not been closed. This can be done by calling upgrade on the Weak held by BreakLoop.

Since no lock is introduced, the overhead is minimal, just a deref of Arc. It might require some care though to make sure the Arc becomes invalid before pcap_close is called to avoid a race condition where breakloop is called after pcap_close is called but before the Arc is dropped.

What do you think?

[Wojtek242]

And I forgot to mention - the interior mutability of the pointer is provided by the fact that NonNull and pointers implement Copy.

[cai-william]

Thanks @Stargateur for making the changes,

I pulled across @Stargateur's changes from their PR #391 fixing a small typo I left in and removing the reference of "poisoned lock" in the commit message.

Tested the changes on Linux for my use case and it works nicely.

LGTM

[FeldrinH]

I'm not sure how relevant it is, but I believe BreakLoop::breakloop as currently implemented cannot be safely called inside signal handlers. If breakloop is called immediately before the original capture goes out of scope then the breakloop Arc will be the last reference to the handle, which means that the handle is dropped and pcap_close is called inside the signal handler, which AFAIK is unsafe.

Might not be a blocker, but at least the docstring should probably be updated to mention this.

[cai-william]

I'm not sure if this is correct but my understanding is that when we call BreakLoop::breakloop() it upgrades the Weak reference to a Strong reference to the Handle that guarantees that pcap_close is not called even if the Capture drops its reference.

EDIT: Oh sorry I misread that comment please continue

[FeldrinH]

Yes, it guarantees that pcap_close is not called when Capture drops the reference. Instead pcap_close will be called when breakloop drops the (strong) reference. If breakloop is called from a signal handler then this is a problem, because calling pcap_close from a signal handler is unsafe.

[FeldrinH]

PS: For what it's worth, my original breakloop using Arc<RwLock> should be safe to call in a signal handler under all circumstances.

[cai-william]

I think the pcap_close should be safe to call from a different thread given no other thread is using pcap_t which our reference count should guarantee.

In terms of calling pcap_close directly in an OS signal handler I agree that it is unsafe, I'm also not sure if RwLock would work in that case either.

I can add a disclaimer that BreakLoop::breakloop() should not be used in a signal handler, and suggest that the signal handler should instead defer the execution of BreakLoop::breakloop() to a thread.

[Stargateur]

Impossible, if a signal is receive during the closing the reference would already be invalid, Arc is atomic. It's impossible for a signal handle to be the one who get the last reference.

[Wojtek242]

I think the pcap_close should be safe to call from a different thread given no other thread is using pcap_t which our reference count should guarantee.

In terms of calling pcap_close directly in an OS signal handler I agree that it is unsafe, I'm also not sure if RwLock would work in that case either.

I can add a disclaimer that BreakLoop::breakloop() should not be used in a signal handler, and suggest that the signal handler should instead defer the execution of BreakLoop::breakloop() to a thread.

Indeed, I don't think either solution is safe in a signal handler. I like your suggestion though. The only other option is to completely remove Arc, mark the breakloop method unsafe and document that it is up to the caller to ensure that the capture exists at the time of calling. Although that would slightly ruin the point of having this crate :-).

@cai-william, I will let you decide which solution to go for in the end. Please add the comment using the Safety pattern present in the other doc strings regardless of which option you go for.

[Wojtek242]

Impossible, if a signal is receive during the closing the reference would already be invalid, Arc is atomic. It's impossible for a signal handle to be the one who get the last reference.

It is possible. Consider the case where breakloop is called and it has just finished the upgrade call, but has not yet done the pcap_breakloop call but at the same time, in another thread the capture is dropped before pcap_breakloop is called. This will result in the breakloop's upgraded Arc being the last reference and thus as soon as it goes out of scope, it will call pcap_close.

[Wojtek242]

A few additional minor comments to tidy up the PR before it is ready for merging. Thanks a lot for your work!

[cai-william]

@cai-william, I will let you decide which solution to go for in the end. Please add the comment using the Safety pattern present in the other doc strings regardless of which option you go for.

Yeah, I think we'll stick to the changes @Stargateur proposed in addition to your suggested changes and the Safety comment.

I can imagine the signal handler would work in a single threaded application, where the signal handler can interrupt the thread's execution and do breakloop() without worrying about Capture dropping.

For multi-threaded applications they can follow the suggestion and have the signal handler defer the task to a thread.

[FeldrinH]

In terms of calling pcap_close directly in an OS signal handler I agree that it is unsafe, I'm also not sure if RwLock would work in that case either.

I'm reasonably sure the RwLock implementation is safe to call in a signal handler. Importantly, it uses try_read, so if the lock can't be aquired immediately then it just assumes that the handle is being closed and does nothing. Importantly it never blocks, so no blocking system calls are made during breakloop.

But I agree that this type of reasoning is iffy in general, since Rust std doesn't really document what is safe to call in a signal handler.

[Stargateur]

Impossible, if a signal is receive during the closing the reference would already be invalid, Arc is atomic. It's impossible for a signal handle to be the one who get the last reference.

It is possible. Consider the case where breakloop is called and it has just finished the upgrade call, but has not yet done the pcap_breakloop call but at the same time, in another thread the capture is dropped before pcap_breakloop is called. This will result in the breakloop's upgraded Arc being the last reference and thus as soon as it goes out of scope, it will call pcap_close.

oh yeah in a multithreading context... well, honestly I dislike both solution anyway... just the simple Arc is way more maintainable that a if handle_valid... we could scope all use of handle into a closure that will always check if handle is valid... but again that will cost some perf.

Honestly signal is so... annoying and limited. Generally people should only do the most basic thing ever in signal handling like changing a flag to true.

[Wojtek242]

In terms of calling pcap_close directly in an OS signal handler I agree that it is unsafe, I'm also not sure if RwLock would work in that case either.

I'm reasonably sure the RwLock implementation is safe to call in a signal handler. Importantly, it uses try_read, so if the lock can't be aquired immediately then it just assumes that the handle is being closed and does nothing. Importantly it never blocks, so no blocking system calls are made during breakloop.

But I agree that this type of reasoning is iffy in general, since Rust std doesn't really document what is safe to call in a signal handler.

Intuitively, I would agree. However, if you look at the list of methods safe to call from a signal handler (https://man7.org/linux/man-pages/man7/signal-safety.7.html) you will not find pthread_rwlock_tryrdlock on the list. While RwLock may have a different implementation that only uses signal-handler-safe methods we cannot tell without looking at the code so comparing to its pthread equivalent is a decent guess.

[Wojtek242]

Impossible, if a signal is receive during the closing the reference would already be invalid, Arc is atomic. It's impossible for a signal handle to be the one who get the last reference.

It is possible. Consider the case where breakloop is called and it has just finished the upgrade call, but has not yet done the pcap_breakloop call but at the same time, in another thread the capture is dropped before pcap_breakloop is called. This will result in the breakloop's upgraded Arc being the last reference and thus as soon as it goes out of scope, it will call pcap_close.

oh yeah in a multithreading context... well, honestly I dislike both solution anyway... just the simple Arc is way more maintainable that a if handle_valid... we could scope all use of handle into a closure that will always check if handle is valid... but again that will cost some perf.

Honestly signal is so... annoying and limited. Generally people should only do the most basic thing ever in signal handling like changing a flag to true.

And indeed - that is what pcap_breakloop does - it sets an atomic flag which is why this discussion is happening. Our implementation of break loop can also potentially call pcap_close. By stating that the caller may only use breakloop if they can guarantee that Capture exists is then the same level of usability as the original pcap_breakloop.

[Wojtek242]

@cai-william, there is only last Clippy warning stating that PcapHandle must be Sync. However, it's a warning and I understand that it is indeed trying to warn about a potential issue.

I would add a clippy allow directive for this warning and justify it with a comment that PcapHandle is not Sync, but may be used as if it was Sync under special circumstance (here breakloop which is special) and the Sync correctness is to be provided by the wrapping structure such as BreakLoop.

[Wojtek242]

Great, thanks a lot for your work @cai-william. I will push a new version to crates with this update later today.