rust-pcap / pcap

Rust language pcap library
Apache License 2.0
595 stars 138 forks source link

Clone impl of BpfProgram look wrong #261

Closed Stargateur closed 1 year ago

Stargateur commented 1 year ago

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 https://github.com/rust-pcap/pcap/commit/d61030082869a0c05b711791a092180c09ef8575 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 commented 1 year ago

I did a bit of digging as to who/where this is being used and the original PR that proposed the change is here: https://github.com/rust-pcap/pcap/pull/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.