Re: [Outreachy kernel] [PATCH] Staging: fbtbt: show() methods should use scnprintf()

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Sep 29, 2015 at 09:50:58PM +0200, Ksenija Stanojević wrote:
> Hi,
> 
> On Tue, Sep 29, 2015 at 9:38 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Sep 29, 2015 at 09:15:23PM +0200, Ksenija Stanojevic wrote:
> >> snprintf() may return a value that exceeds its second argument,
> >> show() methods should use scnprintf() instead of snprintf().
> >>
> >> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> >> ---
> >>  drivers/staging/fbtft/fbtft-sysfs.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
> >> index 8d8bd12..36a1f4e 100644
> >> --- a/drivers/staging/fbtft/fbtft-sysfs.c
> >> +++ b/drivers/staging/fbtft/fbtft-sysfs.c
> >> @@ -198,7 +198,7 @@ static ssize_t show_debug(struct device *device,
> >>       struct fb_info *fb_info = dev_get_drvdata(device);
> >>       struct fbtft_par *par = fb_info->par;
> >>
> >> -     return snprintf(buf, PAGE_SIZE, "%lu\n", par->debug);
> >> +     return scnprintf(buf, PAGE_SIZE, "%lu\n", par->debug);
> >
> > They should?  Why?  This shouldn't be needed at all, and really, a
> > simple sprintf() is all that is necessary.  I would just leave this
> > alone, it's fine as-is.
> 
> In kernel documentation :
> https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt
> it states:
> 
> show() must not use snprintf() when formatting the value to be
>   returned to user space. If you can guarantee that an overflow
>   will never happen you can use sprintf() otherwise you must use
>   scnprintf()
> 
> Should I just use sprintf instead?

If you are writing new code for sysfs attributes like these, yes, use
sprintf().  Don't modify existing code that is working just fine with no
errors at all by trying to convert snprintf() to scnprintf() please.

thanks,

greg k-h