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

Clone impl of BpfProgram look wrong #261

Closed
Stargateur opened this issue Aug 16, 2022 · 1 comment · Fixed by #265
Closed

Clone impl of BpfProgram look wrong #261

Stargateur opened this issue Aug 16, 2022 · 1 comment · Fixed by #265

Comments

@Stargateur
Copy link
Contributor

Stargateur commented Aug 16, 2022

It's unclear if this current Clone impl is legal. There is nothing in https://npcap.com/guide/wpcap/pcap_compile.html or https://npcap.com/guide/wpcap/pcap_freecode.html that said the pointer is allocated with malloc, this mean that if pcap doesn't use malloc, freecode would not be aware we used malloc to clone the struct. This code is from a #[derive(Clone)] that was obviusly not working d610300 I think there is no need to have a Clone for this struct. I think the best would be to remove this Clone unless we have more information.

impl Clone for BpfProgram {
    // make a deep copy of the underlying program
    fn clone(&self) -> Self {
        let len = self.0.bf_len as usize;
        let size = len * mem::size_of::<raw::bpf_insn>();
        let storage = unsafe {
            let storage = libc::malloc(size) as *mut raw::bpf_insn;
            ptr::copy_nonoverlapping(self.0.bf_insns, storage, len);
            storage
        };
        BpfProgram(raw::bpf_program {
            bf_len: self.0.bf_len,
            bf_insns: storage,
        })
    }
}
@Wojtek242
Copy link
Collaborator

I did a bit of digging as to who/where this is being used and the original PR that proposed the change is here: #56. The PR points to https://github.com/LTD-Beget/syncookied as the project where it was being used. However, they've been using their own fork of pcap and the project stopped getting updates before pcap got picked up again. However, looking through its code, they never made use of clone anyway.

Therefore, if you think it is wrong, and it indeed looks wrong given that there's pcap_freecode, I'd just remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants