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 detected when crafting packets. #154

Closed ncrumbak closed 5 years ago

ncrumbak commented 5 years ago

The Packet::addLayer calls Packet::insertLayer to add the current layer to the end of the packet. The data from the layer is appended and deleted. The current layer is linked into the packet object as well. When the Packet is deleted the destructor calls destructPacketData which will delete each of the layers if the value of m_IsAllocatedInPacket is true for the layer. The problem is that Packet::insertLayer doesn't set m_IsAllocatedInPacket;

Packet::insertLayer should set m_IsAllocatedInPacket to true for the layer at it attaches so that Packet::destructPacketData can delete all of the layers when the Packet object is deleted.

I noticed this issue when generating a lot (millions) of packets. I ran valgrind and it showed that each of the calls to Packet::addLayer was leaking. I added a line to Packet::insertLayer to set "newLayer->m_IsAllocatedInPacket = true" right after it sets "newLayer->m_Packet = true" and then recompiled the Packet++ lib and TestLeak code. Then I ran the test again with valgrind and no memory leaks were detected.

Linux version 3.0.101-108.13.1.14249.0.PTF-default (geeko@buildhost) (gcc version 4.3.4 [gcc-4_3-branch revision 152973] (SUSE Linux) )

valgrind-3.14.0/vg-in-place --leak-check=full TestLeak

==4405== HEAP SUMMARY: ==4405== in use at exit: 2,880,000 bytes in 40,000 blocks ==4405== total heap usage: 711,776 allocs, 671,776 frees, 66,782,924 bytes allocated ==4405== ==4405== 1,152 (192 direct, 960 indirect) bytes in 3 blocks are definitely lost in loss record 1 of 8 ==4405== at 0x4C29119: operator new(unsigned long) (vg_replace_malloc.c:332) ==4405== by 0x40A653: generate_packet(pcpp::Packet) (testleak.cc:54) ==4405== by 0x40A2A0: main (testleak.cc:18) ==4405== ==4405== 3,168 (640 direct, 2,528 indirect) bytes in 8 blocks are definitely lost in loss record 2 of 8 ==4405== at 0x4C29119: operator new(unsigned long) (vg_replace_malloc.c:332) ==4405== by 0x40A568: generate_packet(pcpp::Packet) (testleak.cc:37) ==4405== by 0x40A2A0: main (testleak.cc:18) ==4405== ==4405== 905,472 (250,320 direct, 655,152 indirect) bytes in 3,129 blocks are definitely lost in loss record 7 of 8 ==4405== at 0x4C29119: operator new(unsigned long) (vg_replace_malloc.c:332) ==4405== by 0x40A5D6: generate_packet(pcpp::Packet) (testleak.cc:45) ==4405== by 0x40A2A0: main (testleak.cc:18) ==4405== ==4405== 1,970,208 (398,656 direct, 1,571,552 indirect) bytes in 6,229 blocks are definitely lost in loss record 8 of 8 ==4405== at 0x4C29119: operator new(unsigned long) (vg_replace_malloc.c:332) ==4405== by 0x40A405: generate_packet(pcpp::Packet) (testleak.cc:29) ==4405== by 0x40A2A0: main (testleak.cc:18) ==4405== ==4405== LEAK SUMMARY: ==4405== definitely lost: 649,808 bytes in 9,369 blocks ==4405== indirectly lost: 2,230,192 bytes in 30,631 blocks ==4405== possibly lost: 0 bytes in 0 blocks ==4405== still reachable: 0 bytes in 0 blocks ==4405== suppressed: 0 bytes in 0 blocks

// TestLeak.cc

include

include

include

include

include

include

include

using namespace pcpp;

void generate_packet(Packet *pkt);

int main(void) { Packet * EthPkt;

for(int i=0; i<10000; i++) { EthPkt = new Packet(1); generate_packet(EthPkt); std::cout << EthPkt->toString(); delete(EthPkt); } }

void generate_packet(Packet mypkt) { std::string srcStr = "00:11:22:33:44:55"; MacAddress srcMac(srcStr.c_str()); std::string dstStr = "66:77:88:99:aa:bb"; MacAddress dstMac(dstStr.c_str()); EthLayer ethLayer = new EthLayer(srcMac, dstMac, PCPP_ETHERTYPE_IP); mypkt->addLayer(ethLayer);

std::string ipSrcStr = "192.168.10.111"; std::string ipDstStr = "192.168.10.112"; IPv4Address ipSrc(ipSrcStr.c_str()); IPv4Address ipDst(ipDstStr.c_str()); IPv4Layer *ip4Layer = new IPv4Layer(ipSrc, ipDst); ip4Layer->getIPv4Header()->ipId = 2000; ip4Layer->getIPv4Header()->timeToLive = 64; mypkt->addLayer(ip4Layer);

uint16_t portSrc = 1234; uint16_t portDst = 5678; TcpLayer *tcpLayer = new TcpLayer(portSrc, portDst); mypkt->addLayer(tcpLayer);

uint16_t payload_size = 256; uint8_t * payload = new uint8_t[payload_size]; for(uint16_t i=0; i<payload_size; i++) payload[i] = i & 0xff;

PayloadLayer *payloadLayer = new PayloadLayer(payload, payload_size, true); mypkt->addLayer(payloadLayer); delete [] payload;

mypkt->computeCalculateFields();

}

seladb commented 5 years ago

Thanks for reporting this issue. This behavior was actually implemented in purpose although I totally understand your point. The idea behind it was that when layers are generated outside of the packet it's the responsibility of their creator to destruct them. Otherwise a code like this would be a problem:

// create a new Ethernet layer
pcpp::EthLayer newEthernetLayer(pcpp::MacAddress("00:50:43:11:22:33"), pcpp::MacAddress("aa:bb:cc:dd:ee"));

// create a new IPv4 layer
pcpp::IPv4Layer newIPLayer(pcpp::IPv4Address(std::string("192.168.1.1")), pcpp::IPv4Address(std::string("10.0.0.1")));
newIPLayer.getIPv4Header()->ipId = htons(2000);
newIPLayer.getIPv4Header()->timeToLive = 64;

// create a new UDP layer
pcpp::UdpLayer newUdpLayer(12345, 53);

// create a new DNS layer
pcpp::DnsLayer newDnsLayer;
newDnsLayer.addQuery("www.ebay.com", pcpp::DNS_TYPE_A, pcpp::DNS_CLASS_IN);

// create a packet with initial capacity of 100 bytes (will grow automatically if needed)
pcpp::Packet newPacket(100);

// add all the layers we created
newPacket.addLayer(&newEthernetLayer);
newPacket.addLayer(&newIPLayer);
newPacket.addLayer(&newUdpLayer);
newPacket.addLayer(&newDnsLayer);

// compute all calculated fields
newPacket.computeCalculateFields();

In this case if the packet's destructor delete the layers which were allocated on the stack, it would cause a memory corruption.

Having said that, I do understand your point, especially when generating millions of packets. Maybe I should add a flag to addLayer() that indicates whether the packet should take ownership of the layer or not. Please let me know what you think

ncrumbak commented 5 years ago

A flag would be great and I think it would solve my issue.

Thanks, Nelson

seladb commented 5 years ago

Sure. How urgent is this for you? Do you think you can do this change and send a PR?

ncrumbak commented 5 years ago

Yes, I can. First time submitting on Github so bear with me.

seladb commented 5 years ago

Thank you so much! Please let me know if you need any help from me. If you're looking for more info on how to contribute to PcapPlusPlus, please check out https://github.com/seladb/PcapPlusPlus/blob/master/CONTRIBUTING.md

seladb commented 5 years ago

Thanks you so much for the fix and the PR! I merged it into master branch and updated the documentation

ncrumbak commented 5 years ago

It was kind of fun figuring out all the steps out to contribute to a github project.

Thanks for your wonderful packet library!!

Nelson

On Tue, Jan 22, 2019 at 2:20 AM seladb notifications@github.com wrote:

Thanks you so much for the fix and the PR! I merged it into master branch and updated the documentation

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/seladb/PcapPlusPlus/issues/154#issuecomment-456309677, or mute the thread https://github.com/notifications/unsubscribe-auth/AFCZR-ntIrUk-4jQdWNBj0gX8OiTLk0uks5vFsnZgaJpZM4Z6PnY .