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

From: Mark Brown <broonie@kernel.org>
To: Aleksandar Gerasimovski 
	<aleksandar.gerasimovski@hitachi-powergrids.com>
Cc: "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>
Subject: Re: [PATCH] spi: tools: fix input string formatting
Date: Fri, 5 Feb 2021 16:20:31 +0000	[thread overview]
Message-ID: <20210205162031.GG4720@sirena.org.uk> (raw)
In-Reply-To: <VI1PR06MB402928E3B0E4C887104BCE22D2B29@VI1PR06MB4029.eurprd06.prod.outlook.com>

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

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.

> 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?

> +		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?

> @@ -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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]