Re: [dpdk-dev] [PATCH] test: fix sprintf with snprintf

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 08 Feb 2019 12:04:22 -0500
Aaron Conole <aconole@redhat.com> wrote:

> Pallantla Poornima <pallantlax.poornima@intel.com> writes:
> 
> > sprintf function is not secure as it doesn't check the length of string.
> > More secure function snprintf is used.
> >
> > Fixes: 727909c592 ("app/test: introduce dynamic commands list")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
> > ---
> >  test/test/commands.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/test/test/commands.c b/test/test/commands.c
> > index 94fbc310e..5aeb35498 100644
> > --- a/test/test/commands.c
> > +++ b/test/test/commands.c
> > @@ -367,6 +367,8 @@ int commands_init(void)
> >  	struct test_command *t;
> >  	char *commands, *ptr;
> >  	int commands_len = 0;
> > +	int total_written = 0;
> > +	int count = 0;
> >  
> >  	TAILQ_FOREACH(t, &commands_list, next) {
> >  		commands_len += strlen(t->command) + 1;
> > @@ -378,7 +380,10 @@ int commands_init(void)
> >  
> >  	ptr = commands;
> >  	TAILQ_FOREACH(t, &commands_list, next) {
> > -		ptr += sprintf(ptr, "%s#", t->command);
> > +		count = snprintf(ptr, commands_len - total_written - 1, "%s#",
> > +				t->command);
> > +		ptr += count;  
> 
> This code is wrong.  From the manpage:
> 
>         Upon successful completion, the snprintf() function shall return
>         the number of bytes that would be written to s had n been
>         sufficiently large excluding the terminating null byte.
> 
> This code you've placed will improperly increment the number of bytes
> taken, since you don't actually check it.
> 
> Additionally, the correct size is calculated in the preceeding blocks,
> and then the appropriately sized block is allocated.  It doesn't make
> any sense to make the change this way.
> 
> If you are intent on changing this code, I suggest something like the
> following (completely untested code).  The rte_xsprintf() function can
> be used in other areas where you're proposing these refactors (but again
> see my earlier comments about whether these are actual concerns, or just
> 'I dislike the sprintf call').

I agree snprintf is dangerous.
Inventing another routine is not really helping much.
New code for printing should not be inline (because it isn't performance critical).

Why not just rewrite the code in the test to use better string management