> 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.
--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;
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
