On Mon, 2024-02-19 at 15:23 +0000, Lee Jones wrote:
> On Sat, 10 Feb 2024, James Bottomley wrote:
>
> > On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> > > On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > Thanks for your patch!
> > > >
> > > > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@xxxxxxxxxx>
> > > > wrote:
> > > > > There is a general misunderstanding amongst engineers that
> > > > > {v}snprintf() returns the length of the data *actually*
> > > > > encoded into the destination array. However, as per the C99
> > > > > standard {v}snprintf() really returns the length of the data
> > > > > that *would have been* written if there were enough space for
> > > > > it. This misunderstanding has led to buffer-overruns in the
> > > > > past. It's generally considered safer to use the
> > > > > {v}scnprintf() variants in their place (or even sprintf() in
> > > > > simple cases). So let's do that.
> > > >
> > > > Confused... The return value is not used at all?
> > >
> > > Future proofing. The idea of the effort is to rid the use
> > > entirely.
> > >
> > > - Usage is inside a sysfs handler passing PAGE_SIZE as the size
> > > - s/snprintf/sysfs_emit/
> > > - Usage is inside a sysfs handler passing a bespoke value as the
> > > size
> > > - s/snprintf/scnprintf/
> > > - Return value used, but does *not* care about overflow
> > > - s/snprintf/scnprintf/
> > > - Return value used, caller *does* care about overflow
> > > - s/snprintf/seq_buf/
> > > - Return value not used
> > > - s/snprintf/scnprintf/
> > >
> > > This is the final case.
> >
> > To re-ask Geert's question: the last case can't ever lead to a bug
> > orproblem, what value does churning the kernel to change it
> > provide? As Finn said, if we want to deprecate it as a future
> > pattern, put it in checkpatch.
>
> Adding this to checkpatch is a good idea.
>
> What if we also take Kees's suggestion and hit all of these found in
> SCSI in one patch to keep the churn down to a minimum?
That doesn't fix the churn problem because you're still changing the
source. For ancient drivers, we keep the changes to a minimum to avoid
introducing inadvertent bugs which aren't discovered until months
later. If there's no actual bug in the driver, there's no reason to
change the code.
Regards,
James
