search

seladb / PcapPlusPlus

PcapPlusPlus is a multiplatform C++ library for capturing, parsing and crafting of network packets. It is designed to be efficient, powerful and easy to use. It provides C++ wrappers for the most popular packet processing engines such as libpcap, Npcap, WinPcap, DPDK, AF_XDP and PF_RING.
https://pcapplusplus.github.io/
The Unlicense
2.75k stars 674 forks source link

Memory leak in IPcapDevice::setFilter(std::string filterAsString)? #166

Closed ECquadrat closed 5 years ago

ECquadrat commented 5 years ago

First of all, thank you a lot for this great library! It eases the handling of libpcap a lot!

Unfortunately I encountered a possible issue. When using valgrind memory analyzer on my Linux application it reports memory leaks to me. Debugging into this issue it looks like there is a call to pcap_freecode(prog) missing in pcpp::IPcapDevice::setFilter(std::string filterAsString).

If I correctly understand the manpage of libpcap (https://www.tcpdump.org/manpages/pcap_freecode.3pcap.html) then a call to pcap_freecode() is required after a compiled program struct has been made a filter program with pcap_setfilter().

It looks like the same question has already been documented by guyharris in 2013: https://github.com/the-tcpdump-group/libpcap/issues/55

BTW: I used Pcap++ V18.08 and libpcap V1.9.0.

Best regards, Marc

Output of the memory checker: ==95730== Memcheck, a memory error detector ==95730== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==95730== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info ==95730== Command: /home/jenkins/workspace/ches_branches_Feature_2022/build/x86.toolchain.cmake_x86_64/Debug/deploy/bin/test_App --gtest_output=xml:/home/jenkins/workspace/ches_branches_Feature_2022/build/x86.toolchain.cmake_x86_64/Debug/googletest_reports/test_App.xml ==95730== Parent PID: 95437 ==95730== ==95730== HEAP SUMMARY: ==95730== in use at exit: 528 bytes in 3 blocks ==95730== total heap usage: 36,614 allocs, 36,611 frees, 5,530,629 bytes allocated ==95730== ==95730== 80 bytes in 1 blocks are definitely lost in loss record 1 of 3 ==95730== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==95730== by 0x8C5CCBE: icode_to_fcode (optimize.c:2330) ==95730== by 0x8C49B95: pcap_compile (gencode.c:758) ==95730== by 0x53371F4: pcpp::IPcapDevice::setFilter(std::cxx11::basic_string<char, std::char_traits, std::allocator >) (PcapDevice.cpp:23) ==95730== by 0x715F6E: test_app::PcapReplay::setFilter(pcpp::GeneralFilter) (PcapReplay.cpp:323) ==95730== ==95730== 192 bytes in 1 blocks are definitely lost in loss record 2 of 3 ==95730== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==95730== by 0x8C5CCBE: icode_to_fcode (optimize.c:2330) ==95730== by 0x8C49B95: pcap_compile (gencode.c:758) ==95730== by 0x53371F4: pcpp::IPcapDevice::setFilter(std::__cxx11::basic_string<char, std::char_traits, std::allocator >) (PcapDevice.cpp:23) ==95730== by 0x715F6E: test_app::PcapReplay::setFilter(pcpp::GeneralFilter) (PcapReplay.cpp:323) ==95730== ==95730== 256 bytes in 1 blocks are definitely lost in loss record 3 of 3 ==95730== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==95730== by 0x8C5CCBE: icode_to_fcode (optimize.c:2330) ==95730== by 0x8C49B95: pcap_compile (gencode.c:758) ==95730== by 0x53371F4: pcpp::IPcapDevice::setFilter(std::cxx11::basic_string<char, std::char_traits, std::allocator >) (PcapDevice.cpp:23) ==95730== by 0x715F6E: test_app::PcapReplay::setFilter(pcpp::GeneralFilter*) (PcapReplay.cpp:323) ==95730== ==95730== LEAK SUMMARY: ==95730== definitely lost: 528 bytes in 3 blocks ==95730== indirectly lost: 0 bytes in 0 blocks ==95730== possibly lost: 0 bytes in 0 blocks ==95730== still reachable: 0 bytes in 0 blocks ==95730== suppressed: 0 bytes in 0 blocks

seladb commented 5 years ago

I think you're right, there might be a missing call to pcap_freecode() there. You probably noticed it's taking an object of type bpf_program * as a parameter which is currently not saved anywhere. That means we also need to save this pointer as another member in the class.

Do you think you can make that fix and submit a PR?

ECquadrat commented 5 years ago

Hi!

I've no experience with git, so I think it would be best if someone with experiences may do it because it needs way less time.

Are you sure you have to store the pointer as member in the class? The manpage sounded to me like the program memory may be copied inside the pcap_setFilter() method.

Regards, Marc

Am 8. März 2019 09:56:52 MEZ schrieb seladb notifications@github.com:

I think you're right, there might be a missing call to pcap_freecode() there. You probably noticed it's taking an object of type bpf_program * as a parameter which is currently not saved anywhere. That means we also need to save this pointer as another member in the class.

Do you think you can make that fix and submit a PR?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/seladb/PcapPlusPlus/issues/166#issuecomment-470854428

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

seladb commented 5 years ago

Git is pretty easy to learn and I can guide you through it, but of course I can also take it. Please let me know what you prefer.

The reason I think we need to store bpf_program* as a member in the class is because pcap_freecode() takes it as a parameter: https://www.tcpdump.org/manpages/pcap_freecode.3pcap.html

To me it also makes sense that this pointer is stored inside of pcap_setfilter() but in that case they could also call pcap_freecode() internally... I'll check in libpcap code to see how pcap_setfilter() is implemented

ECquadrat commented 5 years ago

Currently I have not that much time left. So I would prefer not doing it. :-)

Am 9. März 2019 19:45:55 MEZ schrieb seladb notifications@github.com:

Git is pretty easy to learn and I can guide you through it, but of course I can also take it. Please let me know what you prefer.

The reason I think we need to store bpf_program* as a member in the class is because pcap_freecode() takes it as a parameter: https://www.tcpdump.org/manpages/pcap_freecode.3pcap.html

To me it also makes sense that this pointer is stored inside of pcap_setfilter() but in that case they could also call pcap_freecode() internally... I'll check in libpcap code to see how pcap_setfilter() is implemented

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/seladb/PcapPlusPlus/issues/166#issuecomment-471210786

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

seladb commented 5 years ago

No worries I can take it. Can you share the code that shows the memory leak?

seladb commented 5 years ago

I've made a fix in the dev branch. Can you please get the latest code from dev and run the memory leak test again?

dev is equal to master, the only difference is this commit.

Please let me know

UPDATE: I made some more changes in dev branch on top of this fix, but they should not affect this fix. Please test in dev branch and let me know

seladb commented 5 years ago

I pushed the fix to master, please verify

ECquadrat commented 5 years ago

Hi again, sorry for the delay, but there have been some additional changes with moving 3rd party files which needed an update of my cmake build environment, too... I've tested now with rev. eeae34e and the memory leak is no more present. Great! :+1:

seladb commented 5 years ago

That's great, I'm glad to hear! I'm closing the issue now, please reopen if needed