RE: [PATCH] spi: tools: fix input string formatting

[Aleksandar Gerasimovski <aleksandar.gerasimovski@xxxxxxxxxxxxxxxxxxxxxx>]

Hi Mark,

Thanks for the feedback, see inline:

-----Original Message-----
From: Mark Brown <broonie@xxxxxxxxxx> 
Sent: Freitag, 5. Februar 2021 17:21
To: Aleksandar Gerasimovski <aleksandar.gerasimovski@xxxxxxxxxxxxxxxxxxxxxx>
Cc: linux-spi@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] spi: tools: fix input string formatting

On Fri, Feb 05, 2021 at 08:04:10AM +0000, Aleksandar Gerasimovski wrote:
> The actual unescape implementation has two bugs:

This is two bugs with two separate fixes, it should be two separate patches.
AG: Ok, I can provide two patches if needed.

> 1. quotation marks from the input string are not removed and are sent
>   to the spidev, e.g: input string: \"\\xFE\\x01\" will be sent to the
>   spidev as 0x22 0xfe 0x01 0x22

It is not clear to me what the issue you see here is - could you be more specific about where you see this input string and why you believe that the handling is incorrect?  After going through the shell the above will be

	"\xFE\x01"

which includes quotation marks which then get sent on to the device.

>  /*
>   *  Unescape - process hexadecimal escape character
> - *      converts shell input "\x23" -> 0x23
> + *      converts shell input "\\x23" -> 0x23
>   */

This is changing the documented input format and not mentioned in the changelog?
AG: Ok my bad!

> +		if (*src == '"') {
> +			src++;
> +			continue;
> +		}
>  		if (*src == '\\' && *(src+1) == 'x') {
>  			match = sscanf(src + 2, "%2x", &ch);
>  			if (!match)

This just appears to ignore quotes which isn't at all what I'd expect?
AG: to be sure we understand each other, you expect quotes to be sent to spi as well? That's expected by design behavior?
Is there any possibility to avoid sending them then?

> @@ -108,6 +112,9 @@ static int unescape(char *_dst, char *_src, size_t len)
>  			src += 4;
>  			*dst++ = (unsigned char)ch;
>  		} else {
> +			match = sscanf(src, "%2d", &ch);
> +			if (!match)
> +				pabort("malformed input string");
>  			*dst++ = *src++;

This appears to be requiring that anything passed into unescape() be a number which isn't something we'd obviously want?  I'd expect the function to unescape things, not to do other random validation which may or may not be appropriate in context.
AG: so by design is expected that everything is accepted here, e.g \"1234qwert\\xde\\xad\"? If yes than I understood this tool wrongly.