[ovs-dev] [PATCH] ofproto-dpif-ipfix: Fix severe memory leak in ipfix_send_template_msgs().

[Justin Pettit]

> On Jun 1, 2017, at 8:46 PM, Ben Pfaff <blp at 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 at 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 at oracle.com>
>>> Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762995&defectInstanceId=4304799&mergedDefectId=180398
>>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>> 
>> Acked-by: Justin Pettit <jpettit at 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.

--Justin


-=-=-=-=-=-=-=-=-=-

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;
 }