| Message ID | BB7A7670-71D9-4BFA-8D66-65287F6CC6BD@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jun 27, 2017 at 11:54:08PM -0700, Justin Pettit wrote: > > > On Jun 1, 2017, at 8:46 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Thu, Jun 01, 2017 at 05:11:37PM -0700, Justin Pettit wrote: > >> > >>> On May 26, 2017, at 9:14 PM, Ben Pfaff <blp@ovn.org> 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 <romain.lenglet@oracle.com> > >>> Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762995&defectInstanceId=4304799&mergedDefectId=180398 > >>> Signed-off-by: Ben Pfaff <blp@ovn.org> > >> > >> Acked-by: Justin Pettit <jpettit@ovn.org> > > > > 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 <blp@ovn.org> 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 <blp@ovn.org> Signed-off-by: Justin Pettit <jpettit@ovn.org> commit 56247a6022ebfa5e55d251f0c43f94a4cb7b0e5b Author: Ben Pfaff <blp@ovn.org> 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 <blp@ovn.org>
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index f8c7ad906acc..5abeba656b4d 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -1311,8 +1311,6 @@ ipfix_send_template_msg(const struct collectors *collectors, tx_errors = ipfix_send_msg(collectors, msg); - dp_packet_uninit(msg); - return tx_errors; }