Skip to content
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

Add Handle that own pointer and call C fn to free a pointer #260

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/handle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#![allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why allow dead code? If as_ref and as_mut are not needed, they're not needed so I'd remove them. They can be added later when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendqueue.rs use them, I could change the code of sendqueue or add cfg feature gate just for them, I didn't like any of the solution.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, leave a comment explaining this then.


use std::ptr::NonNull;

/// MUST NOT IMPLEMENT COPY or CLONE
/// This struct OWN the pointer
/// and have a F implementation that will call F to free the pointer.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite comment to be more formal and clearer. I suggest.

`Handle` MUST NOT implement `Copy` or `Clone` because it assumes it's the sole owner of the pointer and it will free it upon `Drop`ing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and it should be a normal comment (i.e. //) and not a documentation comment (i.e. ///).

#[derive(Debug)]
pub struct Handle<T> {
handle: NonNull<T>,
drop: unsafe extern "C" fn(*mut T),
}

impl<T> Handle<T> {
pub fn new(handle: NonNull<T>, drop: unsafe extern "C" fn(*mut T)) -> Self {
Self { handle, drop }
}

pub fn as_ptr(&self) -> *mut T {
self.handle.as_ptr()
}

pub unsafe fn as_ref(&self) -> &T {
self.handle.as_ref()
}

pub unsafe fn as_mut(&mut self) -> &mut T {
self.handle.as_mut()
}
}

impl<T> Drop for Handle<T> {
fn drop(&mut self) {
unsafe { (self.drop)(self.as_ptr()) }
}
}
30 changes: 12 additions & 18 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ use windows_sys::Win32::Networking::WinSock::{AF_INET, AF_INET6, SOCKADDR_IN, SO
#[cfg(target_os = "windows")]
use windows_sys::Win32::Foundation::HANDLE;

mod handle;
use handle::Handle;
mod raw;
#[cfg(windows)]
pub mod sendqueue;
Expand Down Expand Up @@ -818,7 +820,7 @@ impl State for Dead {}
/// ```
pub struct Capture<T: State + ?Sized> {
nonblock: bool,
handle: NonNull<raw::pcap_t>,
handle: Handle<raw::pcap_t>,
_marker: PhantomData<T>,
}

Expand All @@ -828,10 +830,10 @@ pub struct Capture<T: State + ?Sized> {
unsafe impl<T: State + ?Sized> Send for Capture<T> {}

impl<T: State + ?Sized> From<NonNull<raw::pcap_t>> for Capture<T> {
fn from(handle: NonNull<raw::pcap_t>) -> Self {
fn from(pcap_t: NonNull<raw::pcap_t>) -> Self {
Capture {
nonblock: false,
handle,
handle: Handle::new(pcap_t, raw::pcap_close),
_marker: PhantomData,
}
}
Expand Down Expand Up @@ -1425,12 +1427,6 @@ impl<T: Activated + ?Sized> AsRawFd for SelectableCapture<T> {
}
}

impl<T: State + ?Sized> Drop for Capture<T> {
fn drop(&mut self) {
unsafe { raw::pcap_close(self.handle.as_ptr()) }
}
}

impl<T: Activated> From<Capture<T>> for Capture<dyn Activated> {
fn from(cap: Capture<T>) -> Capture<dyn Activated> {
unsafe { mem::transmute(cap) }
Expand All @@ -1439,7 +1435,7 @@ impl<T: Activated> From<Capture<T>> for Capture<dyn Activated> {

/// Abstraction for writing pcap savefiles, which can be read afterwards via `Capture::from_file()`.
pub struct Savefile {
handle: NonNull<raw::pcap_dumper_t>,
handle: Handle<raw::pcap_dumper_t>,
}

// Just like a Capture, a Savefile is safe to Send as it encapsulates the entire lifetime of
Expand Down Expand Up @@ -1470,14 +1466,10 @@ impl Savefile {
}

impl From<NonNull<raw::pcap_dumper_t>> for Savefile {
fn from(handle: NonNull<raw::pcap_dumper_t>) -> Self {
Savefile { handle }
}
}

impl Drop for Savefile {
fn drop(&mut self) {
unsafe { raw::pcap_dump_close(self.handle.as_ptr()) }
fn from(pcap_dumper_t: NonNull<raw::pcap_dumper_t>) -> Self {
Savefile {
handle: Handle::new(pcap_dumper_t, raw::pcap_dump_close),
}
}
}

Expand Down Expand Up @@ -1520,6 +1512,8 @@ fn test_struct_size() {

#[repr(transparent)]
pub struct BpfInstruction(raw::bpf_insn);

// Be aware of the Drop impl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per #261, please submit a PR to remove this, then rebase and we won't need this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately I can't use Handle for remove the BpfProgram Drop impl. I will see what I can do.

#[repr(transparent)]
pub struct BpfProgram(raw::bpf_program);

Expand Down
13 changes: 3 additions & 10 deletions src/sendqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ use std::ptr::NonNull;

use libc::c_uint;

use crate::handle::Handle;
use crate::raw;
use crate::Error;
use crate::{Active, Capture};

pub struct SendQueue(NonNull<raw::pcap_send_queue>);
pub struct SendQueue(Handle<raw::pcap_send_queue>);

pub enum Sync {
Off = 0,
Expand All @@ -19,7 +20,7 @@ impl SendQueue {
let squeue = unsafe { raw::pcap_sendqueue_alloc(memsize) };
let squeue = NonNull::new(squeue).ok_or(Error::InsufficientMemory)?;

Ok(Self(squeue))
Ok(Self(Handle::new(squeue, raw::pcap_sendqueue_destroy)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with Capture and Savefile, please implement a From<NonNull<raw::pcap_send_queue>> trait and use that.

}

pub fn maxlen(&self) -> c_uint {
Expand Down Expand Up @@ -86,11 +87,3 @@ impl SendQueue {
unsafe { self.0.as_mut() }.len = 0;
}
}

impl Drop for SendQueue {
fn drop(&mut self) {
unsafe {
raw::pcap_sendqueue_destroy(self.0.as_ptr());
}
}
}