From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-f195.google.com (mail-pl1-f195.google.com [209.85.214.195]) by dpdk.org (Postfix) with ESMTP id B43881B49B for ; Wed, 3 Apr 2019 17:27:19 +0200 (CEST) Received: by mail-pl1-f195.google.com with SMTP id t16so6334278plo.0 for ; Wed, 03 Apr 2019 08:27:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=IzQTYub1eqF0BOcAodHo1cthIWPn93ftjF0koNyymg0=; b=ZF5ZUVQqOZVbYYiqG8cjJ4aj5oHI+7wroIfhdq0QKeR6QhCPIckoD4VG6ZEcj3PXYF /WqzVNCzXlfjbQxkYtXA2Zlo8mC8yTO7A2iMzSTszITG2Iet/BeqoBjLOxJFb22gJ9np p8Dn/Ce1VeRqYwdVmpAQD7TA/K4XYe2dwslQ0VpepZM1T0JxThNs+wftxTqhKm7td7YE vyj1AbUNBc5ijeuAT4oy15Xfi48eM7oCx7lP+vzwA/5KxwsTA5KowOHNyqzNGy9SFvls DkI/qeP8ywt2slF1eLFQ+CGuOOsiH50jlJsB7sxybm2Q5DxK2coDgAuihU8nofUP8+wJ fKNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=IzQTYub1eqF0BOcAodHo1cthIWPn93ftjF0koNyymg0=; b=tkEagNl6fd+EzVRbLvnLyRsxqNtkUt9Xk+UFKKCG1PrTzI9T6A7nyDWgsbeiheqAQo UqiKSgyX/0+0Dw4j0Oq9dOsnhh/6t1ziEfwoepayEb/ekvxCnmaPMMimJdqHT3xCOrhp E+FyvdtLqI1PIL3QDlvZhZp5+4Y/Xx3mbVK6LQTgewTclbOxgFuyIXQrelrPB3WPcKxW 4vlbYf6z2I+6x+dnk5x+DzNnJ6xBIHY4mNAU6A3WwhnH5Y0cWdPHKd6edeJiZwuoQKMu oawjPUbU89viEk3SzotjorfN8B2vXjoEw/qlr8jAdh+p7VmI8Mo0EtfqRB3BdQnY+VJZ hDmA== X-Gm-Message-State: APjAAAW8CWffWFVNbuAINGZqVwL4YVINTQWLft6jZuCW9wrVgaEYhXdr u+qbpgf165dxy0iss2h1Uc1uUA== X-Google-Smtp-Source: APXvYqyZ8WOGUr0hJ9gCquKArFvBC7R/PgXJo25+gYpNS0Ky9gaE66XfcisPb394IuPJi+vg9+WtLA== X-Received: by 2002:a17:902:16a4:: with SMTP id h33mr536272plh.23.1554305238529; Wed, 03 Apr 2019 08:27:18 -0700 (PDT) Received: from shemminger-XPS-13-9360 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id p2sm1142928pfb.80.2019.04.03.08.27.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 03 Apr 2019 08:27:18 -0700 (PDT) Date: Wed, 3 Apr 2019 08:27:14 -0700 From: Stephen Hemminger To: "Wiles, Keith" Cc: "Richardson, Bruce" , "dev@dpdk.org" Message-ID: <20190403082714.377f372a@shemminger-XPS-13-9360> In-Reply-To: <5F7D9EB2-AD8D-475B-852C-D8B25475F25C@intel.com> References: <20190403144505.46234-1-bruce.richardson@intel.com> <5F7D9EB2-AD8D-475B-852C-D8B25475F25C@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 0/5] clean up snprintf use for string copying X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2019 15:27:20 -0000 On Wed, 3 Apr 2019 14:57:49 +0000 "Wiles, Keith" wrote: > > On Apr 3, 2019, at 9:45 AM, Bruce Richardson wrote: > > > > There are quite a few instances remaining in DPDK where snprintf is being > > used for string copying. These were not being picked up by our existing > > coccinelle script, but that can be fixed by editing the script and running > > it against our code. In the process a bug was found and fixed in the > > bonding pmd, where we were incorrectly specifiying the buffer length > > parameter to snprintf. > > > > The actual replacement was done in two phases - first replacing all > > instances where only the snprintf line in question needed changing, then > > fixing the other instances where we also needed to add in the header > > include. [Using two stages allowed the header addition to be automated too, > > since we had a list of files where every one needed the header inclusion] > > > > > > Bruce Richardson (5): > > net/bonding: fix buffer length when printing strings > > devtools/cocci: make strlcpy replacement smarter > > devtools/cocci: create safer version of strlcpy script > > replace snprintf with strlcpy without adding extra include > > replace snprintf with strlcpy > > > > Should we not be testing the return values from strlcpy and snprintf, which means we need to create a macro or inline function. We could use a macro and only enable with DEBUG support if we think performance or code size if a problem. > > I am surprised none of the tools are catching these types of problems. > > Not to make Bruce do that change for this patch, but we need to look at it for a later patch IMO. > > Regards, > Keith > In a lot of cases the error checking should be earlier in the input path. Detecting overflow late in making a string is unhelpful. But rejecting input where the name is too long to start with is safer and better. It is one of those cases where static checkers tell you there is a problem but the right solution is farther back in the code, not just right where the error is reported. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 830E3A0679 for ; Wed, 3 Apr 2019 17:27:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 355C11B4A6; Wed, 3 Apr 2019 17:27:21 +0200 (CEST) Received: from mail-pl1-f195.google.com (mail-pl1-f195.google.com [209.85.214.195]) by dpdk.org (Postfix) with ESMTP id B43881B49B for ; Wed, 3 Apr 2019 17:27:19 +0200 (CEST) Received: by mail-pl1-f195.google.com with SMTP id t16so6334278plo.0 for ; Wed, 03 Apr 2019 08:27:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=IzQTYub1eqF0BOcAodHo1cthIWPn93ftjF0koNyymg0=; b=ZF5ZUVQqOZVbYYiqG8cjJ4aj5oHI+7wroIfhdq0QKeR6QhCPIckoD4VG6ZEcj3PXYF /WqzVNCzXlfjbQxkYtXA2Zlo8mC8yTO7A2iMzSTszITG2Iet/BeqoBjLOxJFb22gJ9np p8Dn/Ce1VeRqYwdVmpAQD7TA/K4XYe2dwslQ0VpepZM1T0JxThNs+wftxTqhKm7td7YE vyj1AbUNBc5ijeuAT4oy15Xfi48eM7oCx7lP+vzwA/5KxwsTA5KowOHNyqzNGy9SFvls DkI/qeP8ywt2slF1eLFQ+CGuOOsiH50jlJsB7sxybm2Q5DxK2coDgAuihU8nofUP8+wJ fKNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=IzQTYub1eqF0BOcAodHo1cthIWPn93ftjF0koNyymg0=; b=tkEagNl6fd+EzVRbLvnLyRsxqNtkUt9Xk+UFKKCG1PrTzI9T6A7nyDWgsbeiheqAQo UqiKSgyX/0+0Dw4j0Oq9dOsnhh/6t1ziEfwoepayEb/ekvxCnmaPMMimJdqHT3xCOrhp E+FyvdtLqI1PIL3QDlvZhZp5+4Y/Xx3mbVK6LQTgewTclbOxgFuyIXQrelrPB3WPcKxW 4vlbYf6z2I+6x+dnk5x+DzNnJ6xBIHY4mNAU6A3WwhnH5Y0cWdPHKd6edeJiZwuoQKMu oawjPUbU89viEk3SzotjorfN8B2vXjoEw/qlr8jAdh+p7VmI8Mo0EtfqRB3BdQnY+VJZ hDmA== X-Gm-Message-State: APjAAAW8CWffWFVNbuAINGZqVwL4YVINTQWLft6jZuCW9wrVgaEYhXdr u+qbpgf165dxy0iss2h1Uc1uUA== X-Google-Smtp-Source: APXvYqyZ8WOGUr0hJ9gCquKArFvBC7R/PgXJo25+gYpNS0Ky9gaE66XfcisPb394IuPJi+vg9+WtLA== X-Received: by 2002:a17:902:16a4:: with SMTP id h33mr536272plh.23.1554305238529; Wed, 03 Apr 2019 08:27:18 -0700 (PDT) Received: from shemminger-XPS-13-9360 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id p2sm1142928pfb.80.2019.04.03.08.27.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 03 Apr 2019 08:27:18 -0700 (PDT) Date: Wed, 3 Apr 2019 08:27:14 -0700 From: Stephen Hemminger To: "Wiles, Keith" Cc: "Richardson, Bruce" , "dev@dpdk.org" Message-ID: <20190403082714.377f372a@shemminger-XPS-13-9360> In-Reply-To: <5F7D9EB2-AD8D-475B-852C-D8B25475F25C@intel.com> References: <20190403144505.46234-1-bruce.richardson@intel.com> <5F7D9EB2-AD8D-475B-852C-D8B25475F25C@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 0/5] clean up snprintf use for string copying X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190403152714.PxaAIDCfrCoxNUSwTXYlhszwWKyX4gagZzyIe7Kr1Mw@z> On Wed, 3 Apr 2019 14:57:49 +0000 "Wiles, Keith" wrote: > > On Apr 3, 2019, at 9:45 AM, Bruce Richardson wrote: > > > > There are quite a few instances remaining in DPDK where snprintf is being > > used for string copying. These were not being picked up by our existing > > coccinelle script, but that can be fixed by editing the script and running > > it against our code. In the process a bug was found and fixed in the > > bonding pmd, where we were incorrectly specifiying the buffer length > > parameter to snprintf. > > > > The actual replacement was done in two phases - first replacing all > > instances where only the snprintf line in question needed changing, then > > fixing the other instances where we also needed to add in the header > > include. [Using two stages allowed the header addition to be automated too, > > since we had a list of files where every one needed the header inclusion] > > > > > > Bruce Richardson (5): > > net/bonding: fix buffer length when printing strings > > devtools/cocci: make strlcpy replacement smarter > > devtools/cocci: create safer version of strlcpy script > > replace snprintf with strlcpy without adding extra include > > replace snprintf with strlcpy > > > > Should we not be testing the return values from strlcpy and snprintf, which means we need to create a macro or inline function. We could use a macro and only enable with DEBUG support if we think performance or code size if a problem. > > I am surprised none of the tools are catching these types of problems. > > Not to make Bruce do that change for this patch, but we need to look at it for a later patch IMO. > > Regards, > Keith > In a lot of cases the error checking should be earlier in the input path. Detecting overflow late in making a string is unhelpful. But rejecting input where the name is too long to start with is safer and better. It is one of those cases where static checkers tell you there is a problem but the right solution is farther back in the code, not just right where the error is reported.