Re: [dpdk-dev] [PATCH 0/5] clean up snprintf use for string copying

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Apr 03, 2019 at 03:57:49PM +0100, Wiles, Keith wrote:
> 
> 
> > On Apr 3, 2019, at 9:45 AM, Bruce Richardson <bruce.richardson@intel.com> 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.
> 

Yes, I think in some/many cases we should be checking the return value, but
unfortunately I don't think it's the case that we always should or should
not do so. In some cases, truncation is fine. Therefore, I don't think an
automated solution can work here, and I don't see much point in wrapping
any of these functions in macros, since the action to take on truncation
probably varies from place to place.

> I am surprised none of the tools are catching these types of problems.
> 
Yes. It would be useful to have a tool to flag this, so we can see under
what circumstances it may be a problem.

> Not to make Bruce do that change for this patch, but we need to look at it for a later patch IMO.
> 
Phew! :-)