Add Handle that own pointer and call C fn to free a pointer #260
Conversation
| @@ -0,0 +1,36 @@ | |||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah okay, leave a comment explaining this then.
|
|
||
| /// MUST NOT IMPLEMENT COPY or CLONE | ||
| /// This struct OWN the pointer | ||
| /// and have a F implementation that will call F to free the pointer. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh and it should be a normal comment (i.e. //) and not a documentation comment (i.e. ///).
| @@ -1520,6 +1512,8 @@ fn test_struct_size() { | |||
|
|
|||
| #[repr(transparent)] | |||
| pub struct BpfInstruction(raw::bpf_insn); | |||
|
|
|||
| // Be aware of the Drop impl | |||
There was a problem hiding this comment.
As per #261, please submit a PR to remove this, then rebase and we won't need this comment.
There was a problem hiding this comment.
unfortunately I can't use Handle for remove the BpfProgram Drop impl. I will see what I can do.
| @@ -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))) | |||
There was a problem hiding this comment.
For consistency with Capture and Savefile, please implement a From<NonNull<raw::pcap_send_queue>> trait and use that.
|
Due to significant changes on |
That an idea to avoid fall in trap of Drop impl like in #244.
The purpose is to limit at maximum the risk of double free, with this
Handlethat own pointer it's very unlucky that a double free happen.There is still one Drop impl of
BpfProgrambut this is a little more difficult since the free function ask for a struct not a pointer, this struct use a lot of unsafe, see #261, if we are sure we can malloc to make the clone so we can use free yourself, meaning we could useHandle::new(ptr, free);.With this PR #244 should not introduce UB.