From patchwork Wed Jun 28 06:54:08 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 781466 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wyD6g36vRz9s74 for ; Wed, 28 Jun 2017 16:54:18 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id E7410728; Wed, 28 Jun 2017 06:54:14 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id C501A723 for ; Wed, 28 Jun 2017 06:54:13 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pg0-f66.google.com (mail-pg0-f66.google.com [74.125.83.66]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 579E91A0 for ; Wed, 28 Jun 2017 06:54:12 +0000 (UTC) Received: by mail-pg0-f66.google.com with SMTP id u36so7105591pgn.3 for ; Tue, 27 Jun 2017 23:54:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=MUl79e/605hDAIu519YlaK6YtwhkoRhouYtUV+UHc4o=; b=SANPV0zJrutftel2sBJYr4N7jqfTmsX6jJWAADM1q1FZ5WHuWR/MbmGpDk2AfIfGQL xEmpJJp2lGZr9Ih9JJ0eJRTOsW+77wgoTR/dAFFFeJ6QA77vYWjwMBqf/8VVmakmKhPS lcQEmOtZQ/8J5jbL0wpok1Ta+MoVtPw2fUjr7+8DqIC6DI5br+ntBJ4buABh496IcqA8 524jVDje9HM6SO8Fc4cwMffTNvzodN5Y4WF9a6ADdcAKPOCHvlf/SYi5/xCPDD1jBRYB VnGjgYFgTJafS/4n6g7HnXt6Wcny2Czy/o8yLV8/xC9QbKl4ONFEFYBhaePHGqaBTGM3 SV2A== X-Gm-Message-State: AKS2vOyAq8qZaIsOaF9xAylqDsAeUDmMKbjKamZtJ+BcFul/UblDPsT/ IFZQq9O/M9OxdsiG2vE= X-Received: by 10.98.6.1 with SMTP id 1mr9039834pfg.64.1498632851419; Tue, 27 Jun 2017 23:54:11 -0700 (PDT) Received: from [10.0.1.5] ([98.234.50.139]) by smtp.gmail.com with ESMTPSA id 75sm2646360pfk.113.2017.06.27.23.54.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Jun 2017 23:54:10 -0700 (PDT) Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) From: Justin Pettit In-Reply-To: <20170602034625.GB2820@ovn.org> Date: Tue, 27 Jun 2017 23:54:08 -0700 Message-Id: References: <20170527041421.24341-1-blp@ovn.org> <3DA4591B-5E42-40C6-A03A-7347E0A0F242@ovn.org> <20170602034625.GB2820@ovn.org> To: Ben Pfaff X-Mailer: Apple Mail (2.3273) X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: ovs dev Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-ipfix: Fix severe memory leak in ipfix_send_template_msgs(). X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org > On Jun 1, 2017, at 8:46 PM, Ben Pfaff wrote: > > On Thu, Jun 01, 2017 at 05:11:37PM -0700, Justin Pettit wrote: >> >>> On May 26, 2017, at 9:14 PM, Ben Pfaff 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 >>> Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14762995&defectInstanceId=4304799&mergedDefectId=180398 >>> Signed-off-by: Ben Pfaff >> >> Acked-by: Justin Pettit > > 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; }