From: Denis Efremov <efremov@linux.com> To: Julia Lawall <julia.lawall@inria.fr>, Joe Perches <joe@perches.com> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>, Kees Cook <keescook@chromium.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, Rasmus Villemoes <linux@rasmusvillemoes.dk>, linux-kernel@vger.kernel.org, cocci <cocci@systeme.lip6.fr>, accessrunner-general@lists.sourceforge.net, Alex Dewar <alex.dewar90@gmail.com> Subject: Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs Date: Fri, 28 Aug 2020 00:01:34 +0300 [thread overview] Message-ID: <5853c58e-7d26-2cf9-6cbf-698ecd93cbf9@linux.com> (raw) In-Reply-To: <alpine.DEB.2.22.394.2008272141220.2482@hadrien> Hi all, On 8/27/20 10:42 PM, Julia Lawall wrote: > > > On Thu, 27 Aug 2020, Joe Perches wrote: > >> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote: >>> On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote: >>>> On 27/08/2020 15.18, Alex Dewar wrote: >>>>> On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote: >>>>>> On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote: >>>>>>> On 25/08/2020 00.23, Alex Dewar wrote: >>>>>>>> kernel/cpu.c: don't use snprintf() for sysfs attrs >>>>>>>> >>>>>>>> As per the documentation (Documentation/filesystems/sysfs.rst), >>>>>>>> snprintf() should not be used for formatting values returned by sysfs. Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would like it more than changing snprintf to scnprintf. As for me, I don't like the idea of automated altering of the original logic from bounded snprint to unbouded one with sprintf. [1] https://lkml.org/lkml/2020/8/13/786 Regarding current device_attr_show.cocci implementation, it detects the functions by declaration: ssize_t any_name(struct device *dev, struct device_attribute *attr, char *buf) and I limited the check to: "return snprintf" pattern because there are already too many warnings. Actually, it looks more correct to check for: ssize_t show(struct device *dev, struct device_attribute *attr, char *buf) { <... * snprintf@p(...); ...> } This pattern should also highlight the snprintf calls there we save returned value in a var, e.g.: ret += snprintf(...); ... ret += snprintf(...); ... ret += snprintf(...); return ret; > > Something like > > identifier f; > fresh identifier = "sysfs" ## f; > > may be useful. Let me know if further help is needed. Initially, I wrote the rule to search for DEVICE_ATTR(..., ..., func_name, ...) functions. However, it looks like matching function prototype is enough. At least, I failed to find false positives. I rejected the initial DEVICE_ATTR() searching because I thought that it's impossible to handle DEVICE_ATTR_RO()/DEVICE_ATTR_RW() macroses with coccinelle as they "generate" function names internally with "##". "fresh identifier" should really help here, but now I doubt it's required in device_attr_show.cocci, function prototype is enough. Thanks, Denis
WARNING: multiple messages have this Message-ID (diff)
From: Denis Efremov <efremov@linux.com> To: Julia Lawall <julia.lawall@inria.fr>, Joe Perches <joe@perches.com> Cc: Kees Cook <keescook@chromium.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, Rasmus Villemoes <linux@rasmusvillemoes.dk>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, linux-kernel@vger.kernel.org, Alex Dewar <alex.dewar90@gmail.com>, accessrunner-general@lists.sourceforge.net, cocci <cocci@systeme.lip6.fr> Subject: Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs Date: Fri, 28 Aug 2020 00:01:34 +0300 [thread overview] Message-ID: <5853c58e-7d26-2cf9-6cbf-698ecd93cbf9@linux.com> (raw) In-Reply-To: <alpine.DEB.2.22.394.2008272141220.2482@hadrien> Hi all, On 8/27/20 10:42 PM, Julia Lawall wrote: > > > On Thu, 27 Aug 2020, Joe Perches wrote: > >> On Thu, 2020-08-27 at 15:48 +0100, Alex Dewar wrote: >>> On Thu, Aug 27, 2020 at 03:41:06PM +0200, Rasmus Villemoes wrote: >>>> On 27/08/2020 15.18, Alex Dewar wrote: >>>>> On Thu, Aug 27, 2020 at 09:15:37AM +0200, Greg Kroah-Hartman wrote: >>>>>> On Thu, Aug 27, 2020 at 08:42:06AM +0200, Rasmus Villemoes wrote: >>>>>>> On 25/08/2020 00.23, Alex Dewar wrote: >>>>>>>> kernel/cpu.c: don't use snprintf() for sysfs attrs >>>>>>>> >>>>>>>> As per the documentation (Documentation/filesystems/sysfs.rst), >>>>>>>> snprintf() should not be used for formatting values returned by sysfs. Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would like it more than changing snprintf to scnprintf. As for me, I don't like the idea of automated altering of the original logic from bounded snprint to unbouded one with sprintf. [1] https://lkml.org/lkml/2020/8/13/786 Regarding current device_attr_show.cocci implementation, it detects the functions by declaration: ssize_t any_name(struct device *dev, struct device_attribute *attr, char *buf) and I limited the check to: "return snprintf" pattern because there are already too many warnings. Actually, it looks more correct to check for: ssize_t show(struct device *dev, struct device_attribute *attr, char *buf) { <... * snprintf@p(...); ...> } This pattern should also highlight the snprintf calls there we save returned value in a var, e.g.: ret += snprintf(...); ... ret += snprintf(...); ... ret += snprintf(...); return ret; > > Something like > > identifier f; > fresh identifier = "sysfs" ## f; > > may be useful. Let me know if further help is needed. Initially, I wrote the rule to search for DEVICE_ATTR(..., ..., func_name, ...) functions. However, it looks like matching function prototype is enough. At least, I failed to find false positives. I rejected the initial DEVICE_ATTR() searching because I thought that it's impossible to handle DEVICE_ATTR_RO()/DEVICE_ATTR_RW() macroses with coccinelle as they "generate" function names internally with "##". "fresh identifier" should really help here, but now I doubt it's required in device_attr_show.cocci, function prototype is enough. Thanks, Denis _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
next prev parent reply other threads:[~2020-08-27 21:01 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-24 22:23 [PATCH] usb: atm: don't use snprintf() for sysfs attrs Alex Dewar 2020-08-25 8:12 ` David Laight 2020-08-25 8:17 ` Alex Dewar 2020-08-25 9:07 ` David Laight 2020-08-25 8:24 ` Greg Kroah-Hartman 2020-08-27 16:49 ` Kees Cook 2020-08-27 17:45 ` Greg Kroah-Hartman 2020-08-27 6:42 ` Rasmus Villemoes 2020-08-27 7:15 ` Greg Kroah-Hartman 2020-08-27 13:18 ` Alex Dewar 2020-08-27 13:41 ` Rasmus Villemoes 2020-08-27 14:48 ` Alex Dewar 2020-08-27 16:58 ` Joe Perches 2020-08-27 16:58 ` [Cocci] " Joe Perches 2020-08-27 19:42 ` Julia Lawall 2020-08-27 19:42 ` Julia Lawall 2020-08-27 20:29 ` Joe Perches 2020-08-27 20:29 ` Joe Perches 2020-08-27 21:00 ` Joe Perches 2020-08-27 21:00 ` Joe Perches 2020-08-27 21:29 ` Julia Lawall 2020-08-27 21:29 ` Julia Lawall 2020-08-27 22:03 ` David Laight 2020-08-27 22:03 ` David Laight 2020-08-27 22:11 ` Joe Perches 2020-08-27 22:11 ` Joe Perches 2020-08-27 22:16 ` Kees Cook 2020-08-27 22:16 ` Kees Cook 2020-08-27 21:01 ` Denis Efremov [this message] 2020-08-27 21:01 ` Denis Efremov 2020-08-27 21:36 ` Julia Lawall 2020-08-27 21:36 ` Julia Lawall 2020-08-27 21:44 ` Joe Perches 2020-08-27 21:44 ` Joe Perches 2020-08-27 22:38 ` Denis Efremov 2020-08-27 22:38 ` Denis Efremov 2020-08-27 22:48 ` Joe Perches 2020-08-27 22:48 ` Joe Perches 2020-08-27 22:20 ` Kees Cook 2020-08-27 22:20 ` Kees Cook 2020-08-27 22:45 ` Joe Perches 2020-08-27 22:45 ` Joe Perches 2020-08-28 4:12 ` Joe Perches 2020-08-28 4:12 ` Joe Perches 2020-08-28 7:58 ` Kees Cook 2020-08-28 7:58 ` Kees Cook 2020-08-28 8:10 ` Joe Perches 2020-08-28 8:10 ` Joe Perches 2020-08-28 8:22 ` Joe Perches 2020-08-28 8:22 ` Joe Perches 2020-08-28 7:39 ` David Laight 2020-08-28 7:39 ` David Laight 2020-08-27 21:54 ` David Laight 2020-08-27 21:54 ` [Cocci] " David Laight
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5853c58e-7d26-2cf9-6cbf-698ecd93cbf9@linux.com \ --to=efremov@linux.com \ --cc=accessrunner-general@lists.sourceforge.net \ --cc=alex.dewar90@gmail.com \ --cc=cocci@systeme.lip6.fr \ --cc=gregkh@linuxfoundation.org \ --cc=gustavoars@kernel.org \ --cc=joe@perches.com \ --cc=julia.lawall@inria.fr \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=linux@rasmusvillemoes.dk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.