On Tue, Jun 27, 2017 at 11:54:08PM -0700, Justin Pettit wrote: > > > On Jun 1, 2017, at 8:46 PM, Ben Pfaff <[email protected]> wrote: > > > > On Thu, Jun 01, 2017 at 05:11:37PM -0700, Justin Pettit wrote: > >> > >>> On May 26, 2017, at 9:14 PM, Ben Pfaff <[email protected]> wrote: > >>> > >>> This fixes a seemingly severe memory leak in ipfix_send_template_msgs(). > >>> This function was setting up a buffer with a stub, but only the first 4 > >>> or 8 bytes of the stub were actually used because the "sizeof" call used > >>> to size it was actually getting the size of a pointer. It never freed > >>> the buffer, leaking it. > >>> > >>> Additionally, after this code sent a template message, it started over > >>> from the same undersized stub, leaking another block of memory. > >>> > >>> This commit fixes both problems. > >>> > >>> Found by Coverity. > >>> > >>> CC: Romain Lenglet <[email protected]> > >>> Reported-at: > >>> https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762995&defectInstanceId=4304799&mergedDefectId=180398 > >>> Signed-off-by: Ben Pfaff <[email protected]> > >> > >> Acked-by: Justin Pettit <[email protected]> > > > > You don't happen to see something I"m missing here, do you? This seems > > like an egregious leak. > > From my reading of the code, it doesn't actually represent a leak; it's just > not using the buffer efficiently. I think what happens is: > > 1) ipfix_send_template_msgs() allocates a 1024-byte buffer on the stack. > > 2) ipfix_send_template_msgs() calls ipfix_init_template_msg(), which > sets the "allocated" size to size of a pointer. This is smaller than the > actual amount allocated, so it's safe. However, it means anytime we generate > an IPFIX message, it will cause us to malloc memory because it thinks there > isn't enough space allocated. > > 3) ipfix_send_template_msgs() always calls ipfix_send_template_msg() > after a call to ipfix_init_template_msg(). ipfix_send_template_msg() calls > dp_packet_uninit(), which frees the associated data, so there's no leak. > > Your change makes it so that we only call dp_packet_use_stub() and > dp_packet_uninit() in ipfix_send_template_msgs(), which means that we > properly set "allocated" to 1024 bytes, reuse the buffer each time we send a > packet (and keep using the larger buffer if it was necessary to grow it), and > then free it as the function exits. > > I do think your change had a bug, though, since dp_packet_uninit() was called > from both ipfix_send_template_msg() and when ipfix_send_template_msgs() was > executed, which caused a double-free. This caused four of the unit tests to > fail. I've appended an incremental that allows all the tests to pass. If > you agree with the change, I'll merge that into your original patch and push > it to appropriate branches.
Thanks for reviewing this and spotting the actual problem and fixing the bug that I introduced. The actual problem is much less significant than I thought--a minor inefficiency rather than a severe memory leak. I see that you (accidentally?) committed this with the original, misleading commit message. I think that it's probably best to provide a more accurate commit message. We can't change the Git history, at least not reasonably, so as a second choice I've reverted the patch and then immediately reapplied it with a more accurate commit message. I'm appending the two commits below (without the diffs, since they're not interesting). commit 642c824b36be9bdde1b3370a97221ae3ca5b4e5f Author: Ben Pfaff <[email protected]> Date: Fri May 26 21:14:21 2017 -0700 ofproto-dpif-ipfix: Fix inefficent memory use in ipfix_send_template_msgs(). This fixes inefficient use of memory in ipfix_send_template_msgs(). This function was setting up a buffer with a stub, but only the first 4 or 8 bytes of the stub were actually used because the "sizeof" call used to size it was actually getting the size of a pointer. This meant that every template message was causing a series of allocations and reallocations. This commit fixes the problem. Found by Coverity. Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762995&defectInstanceId=4304799&mergedDefectId=180398 Signed-off-by: Ben Pfaff <[email protected]> Signed-off-by: Justin Pettit <[email protected]> commit 56247a6022ebfa5e55d251f0c43f94a4cb7b0e5b Author: Ben Pfaff <[email protected]> Date: Wed Jul 5 15:42:49 2017 -0700 Revert "ofproto-dpif-ipfix: Fix severe memory leak in ipfix_send_template_msgs()." This reverts commit 4d6f69df54b7d6ec2956875c683a9564cb175662. There is nothing wrong with the commit itself, but the commit message is misleading. The following commit will re-apply it with a corrected commit message. Signed-off-by: Ben Pfaff <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
