Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Sean Anderson <seanga2@gmail.com>]

On 6/29/21 11:13 AM, Wolfgang Denk wrote:
> Dear Sean,
> 
> In message <19b6eeea-2aad-972b-aeeb-8959aab17d7a@gmail.com> you wrote:
>>
>> The issue with this is twofold. First, there is no portable way to
>> construct a va_list from C code. So the likely way to do this would be
>> to set an arbitrary limit, and then just pass the arguments in. E.g.
>> something like
> 
> We already have an argument list: it's what's being passed to the
> "setexpr" command, minus the initial arguments.
> 
>> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc */);
> 
> Why this test on argc?  If it's less than 4, argv[4] should be NULL
> anyway.

	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL,
		 argc >= 5 ? argv[5] : NULL, argc >= 6 ? argv[6] : NULL, /* etc */);

and you keep doing this until you get to whatever number of arguments
you'd like.

> 
>> but of course there is no way to check that the format string matches
>> the correct number of arguments. This is a pretty big footgun.
> 
> You have this problem always when you have user provided format
> strings and arguments.  We don't have to re-invent the wheel here.
> I repeat myself: maybe we should have a look at bash's
> implementation of the printf builtin command?  there I get for
> example this:
> 
> 	$ printf "%d %d %d\n" 3
> 	3 0 0
> 	$ printf "%d %d %d\n" foo bar
> 	-bash: printf: foo: invalid number
> 	-bash: printf: bar: invalid number
> 	0 0 0
>> The other problem is that things like `%d` expect a number and not a
>> string. So you would have to reimplement snprintf anyway so that it
>> expects all of its arguments to be strings, and calls strtoul as
>> appropriate.  And considering that the *printf functions take 5k
>> already, this reimplementation may add a significant amount of code.
>> For this reason, I'd much prefer to just have `hex` and `dec` functions
>> which do the appropriate conversions.
> 
> Eventually the format checking can be kept out of the generic
> *printf() code; it could then be optional/configurable with the
> "fmt" option in the setexpr command.

It's not a "checking" problem. The issue is that "123" cannot
be passed directly to %d. So you have dig into the guts of snprintf
anyway.

--Sean