Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp966917imm; Fri, 13 Jul 2018 09:12:46 -0700 (PDT) X-Google-Smtp-Source: AAOMgpdPMRMnDv2j39qdhq61cbzq+23OTTUdAQSHyYo1HJSM/M9sAIsfKGd+rrVvGovbOdBXn+vB X-Received: by 2002:a17:902:7009:: with SMTP id y9-v6mr6965675plk.217.1531498366056; Fri, 13 Jul 2018 09:12:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1531498366; cv=none; d=google.com; s=arc-20160816; b=LxYfXVLUlFFYkIdsOo0fA3tB9zoJaiog1dL6pKlW3KbDYhgW7Farj07/WrxO7LBlWi IfzFCP871yRnuO4Uc3FmSBQaCx82s6B3GmEmX9C+j/kE8t7d72s0nciV92biebUjeqLK sXUrJeSz0+Ktmp2o9mbpKHp7mBgo6i0kfmTG3JzK093+vP6OQil+2M/qU2XWr1aX19/3 AR9iEW8oLBxJjVL4I0DiafVFRox76D5ZDmDWyc5Th2BmGoht5YEBFutZLKrQX0ekg4Gp /KYxek4Hgd9fCD5LxdjoZiA32QiBOyJFxXUReroCHytN5lS2rQ3FaLjTTfRxBlQsrCw5 qgRA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:message-id:subject:cc:to:from:date :arc-authentication-results; bh=GL5zgSdI+KkCPPKoYnJhRPxKSJ7x9S0K0lyne35bfuU=; b=v4mWBgBxowdVVHpxcnmVHf6MnFrUgJTFMI9RgSgfN1J9VsB5V+MdRON0RqM+dv4BBy KGsZA8Gmj1nmPSmeBKPnWqNuF6X9t/jr2ON5yD33mrNf2Mi5A+Rxmdyp8+57cLnBeIbL esjfZp7Dgt+rQfGoWExf0Ca4odT8G0RuWYVkfgCvglhKCEwy6Jf8AIv757oCtDPGDGBV ig2UQ11IHGbjEO+0rJt/XqhWvhguvKVrfcPmfPssHDooieJwDXPKF7Dp8qUSTFR1MAZ1 C9mEkElpWyzFtPuDh0o3RtulT2OMCiNhuofyPbe4Y8bcTiLDPQoCLHYIs+MCfoOIZU36 VwXQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id bh7-v6si23597759plb.367.2018.07.13.09.12.30; Fri, 13 Jul 2018 09:12:46 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731493AbeGMQ1L (ORCPT + 99 others); Fri, 13 Jul 2018 12:27:11 -0400 Received: from nautica.notk.org ([91.121.71.147]:36771 "EHLO nautica.notk.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729736AbeGMQ1L (ORCPT ); Fri, 13 Jul 2018 12:27:11 -0400 Received: by nautica.notk.org (Postfix, from userid 1001) id BBEC6C009; Fri, 13 Jul 2018 18:11:53 +0200 (CEST) Date: Fri, 13 Jul 2018 18:11:38 +0200 From: Dominique Martinet To: Himanshu Jha , Julia Lawall Cc: Michal Marek , Nicolas Palix , linux-kernel@vger.kernel.org, cocci@systeme.lip6.fr, Ville =?utf-8?B?U3lyasOkbMOk?= , yamada.masahiro@socionext.com Subject: Re: [Cocci] [PATCH 01/18] coccinelle: change strncpy+truncation to strlcpy Message-ID: <20180713161138.GA17418@nautica> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180713091412.GA11250@himanshu-Vostro-3559> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Himanshu Jha wrote on Fri, Jul 13, 2018: > Try this: > > $ spatch -D patch --sp-file strlcopy.cocci --very-quiet drivers/net/wireless/ti/wl1251/acx.c > > --------------------------------------------------------------------- > virtual patch > > @depends on patch@ > expression dest, src, sz; > identifier f; > @@ > > ( > - strncpy( > + strlcpy( > dest, src, sizeof(sz)); > - dest[sizeof(sz) - 1] = '\0'; > | > - strncpy( > + strlcpy( > dest, src, f); > - dest[f - 1] = '\0'; > ) > --------------------------------------------------------------------- > > This eliminates that case because expression is generic metavariable and > it somehow matched whole "min(len, sizeof(...)..", so it better to > divide the rules as done above to be more specific about the matching > pattern. > > I thought to replace "identifier f" with "constant F" but that misses > few cases. My first test started with 'constant sz' as well and I found expression to be better. If I understand this correctly, it just makes sure not to match the 'min(...)' expression so the problem doesn't arise, but it's not really a solution as it is really a chance that this comment comes here for this more complex expression. I'd rather just advise to pay attention to comments and drop the confidence level > Also, it is advised to put a space affer '+/-' Ok, thanks Julia Lawall wrote on Fri, Jul 13, 2018: > For the file drivers/net/wireless/ti/wl1251/acx.c, the following keeps the > comment before the buf update and moves the strlcpy call below it. It > does however drop the comment just before the original call to strncpy. Nice, doing it in two passes does the trick; it even keeps the comment before strncpy if there was no comment in between. I'm sorry to write that after being provided with such a nice work-around though, now that I'm a bit less tired I've had a look at the comments again and it does not make sense to keep the second comment as is -- the whole point of using strlcpy (or now strscpy as per feedback elsewhere) is that the string will be null terminated properly after the call, so I'll stick to the original version. I also see the position does not seem to be useful for the patch phase, spatch automatically only applies only to what was matched earlier in report mode? >>> +-strncpy@p >>> ++strlcpy >>> + (dest, src, sz); >> >> This also came from the example I picked, but if this does not make a >> difference as it sounds like I will update to that. > > Probably not removing something just to add it back would be a good > idea. Actually playing with your example made me realize that removing and re-adding argument does fix the indentation issue in the original code I had noticed for mptctl, so it might actually unexpectedly be a good idea to go in the opposite direction and make coccinelle remove/add arguments in the general case (e.g. if strncpy and strlcpy hadn't been the same length) The jury is still out for this one :) Thanks for all the feedbacks, -- Dominique Martinet