pcap_breakloop() with indefinitely-blocking captures

[qrnch-jan]

There's currently no way to break a blocking pcap receiver loop in the Rust wrapper, and it's not clear how one would go about implementing such a thing (without adding a bunch of unwanted overhead).

Applications can call pcap_breakloop() to request that the loop be terminated, this however does not actually break a blocking receiver, it just sets a flag to indicate that the loop should terminate. Whatever platform-specific mechanism is available to unblock a syscall must be used in conjunction with pcap_breakloop(). On unixy platforms this would typically mean that another thread uses signals (without SA_RESTART) to wake the blocking thread.

Calling pcap_breakloop() from a different thread has some pretty nasty implications -- most notably that pcap* must exist in two threads.

We could add a nice - safe - abstraction to solve all of this, but it would - as mentioned above - involve some unwanted overhead, and beside that it would likely involve some design time.

Another way is to simply allow the application to solve these issues themselves, but it's currently not possible because we don't expose what the application needs.

I suggest that we expose some means for an application to call pcap_breakloop() through some unsafe interface. Either we expose a means to get a raw pcap* and expose a pcap_breakloop(), or we add something like:

struct BreakLoop {
  pcap: *const pcap_t
}
impl BreakLoop {
  unsafe fn trigger(&self) {
    pcap_breakloop(self.pcap);
  }
}

The pcap_breakloop() manpage mentions breaking pcap loops from other threads, so it's designed to allow this.

To be clear, developers can use timeouts and poll a termination request flag; but I think we should offer developers the building blocks to enable a non-polled mechanism for terminating capture loops.

[Stargateur]

https://github.com/rust-pcap/pcap/pull/310 is about adding pcap_loop, since we don't use it in pcap yet I don't understand your issue

[qrnch-jan]

Oh, right -- I thought pcap_loop() was already implemented -- sorry, for some reason I was convinced I had seen it.

We have some legacy C and C++ code that we're looking to port to Rust, and the first step is to do as "straight" port as possible. Some of the projects use pcap_loop on one thread and uses signals & pcap_breakloop() from a different thread to terminate the loop; I want whoever ports the code to be able to do the same in Rust.

So basically, once we have pcap_loop() I'll want pcap_breakloop() as well. :)

[Wojtek242]

As of 1.3.0 pcap_loop is available via the for_each call. I think your simple trigger idea is good. I would only ask to hide it behind a feature as I would hope that eventually somebody can propose the nice abstraction you speak of after having used the simple solution. If you would like to submit a PR for that, it would be welcome!

[FeldrinH]

Applications can call pcap_breakloop() to request that the loop be terminated, this however does not actually break a blocking receiver, it just sets a flag to indicate that the loop should terminate. Whatever platform-specific mechanism is available to unblock a syscall must be used in conjunction with pcap_breakloop().

In more recent versions of libpcap pcap_breakloop can actually wake up the blocked thread in some cases (specifically, this works on Linux when doing network capture; see start of https://www.tcpdump.org/manpages/libpcap-1.10.4/pcap_breakloop.3pcap.html).

310 is about adding pcap_loop, since we don't use it in pcap yet I don't understand your issue

pcap_breakloop is also useful to interrupt a call to pcap_next_ex if there are currently no packets being received (on several platforms, including Linux, pcap_next_ex will block indefinitely if there are no packets received).

[FeldrinH]

I propose adding a lock that protects calls to pcap_breakloop and pcap_close, to ensure they don't overlap and that pcap_breakloop is not called after pcap_close. The implementation would look approximately like this:

pub struct BreakLoop {
    handle: NonNull<raw::pcap_t>,
    handle_valid: Arc<RwLock<bool>>,
}

impl BreakLoop {
    pub fn break(&self) {
        if let Ok(handle_valid) = self.handle_valid.try_read() {
            if *handle_valid {
                // Safety: 
                // handle_valid is true, so the handle is valid before pcap_breakloop
                // handle_valid lock is held for the duration of pcap_breakloop so the handle can't be deallocated while pcap_breakloop is running
                unsafe { raw::pcap_breakloop(self.handle.as_ptr()) };
            }
        }
    }
}

pub struct Capture<T: State + ?Sized> {
    nonblock: bool,
    handle: NonNull<raw::pcap_t>,
    /// Indicates if the allocation for the handle is valid. This is only set to false when this capture is dropped.
    /// It is used to notify other threads that the handle has become invalid.
    handle_valid: Arc<RwLock<bool>>,
    _marker: PhantomData<T>,
}

impl<T: State + ?Sized> Drop for Capture<T> {
    fn drop(&mut self) {
        // Lock handle_valid to ensure that other threads don't try to call any functions on the handle while pcap_close is running
        let mut handle_valid = self.handle_valid.write().unwrap();
        *handle_valid = false;
        // Safety:
        // The handle is valid before pcap_close, because this Capture instance owns the handle and it is only ever closed in this Drop implementation
        // handle_valid is set to false so any functions in other threads will know not to access the handle
        unsafe { raw::pcap_close(self.handle.as_ptr()) }
    }
}

The main drawback as far as I can tell is that BreakLoop.break is not safe to call inside a signal handler (because another signal could interrupt the locking or unlocking operation and corrupt the internal state of the lock). In practice this probably won't matter too much, because writing signal handlers that run custom code is discouraged in Rust due to the large number of potential safety issues surrounding it.

Is this safe and correct? Would appreciate some kind of review from someone more experienced with the internals of this crate.