| Message ID | VI1PR06MB402928E3B0E4C887104BCE22D2B29@VI1PR06MB4029.eurprd06.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: tools: fix input string formatting | expand |
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.
Hi Mark, Thanks for the feedback, see inline: -----Original Message----- From: Mark Brown <broonie@kernel.org> Sent: Freitag, 5. Februar 2021 17:21 To: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com> Cc: linux-spi@vger.kernel.org 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.
On Sat, Feb 06, 2021 at 10:57:04AM +0000, Aleksandar Gerasimovski wrote: > 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? If you don't want quotes to be sent then don't send them - my expectation would be that if you're driving this from the shell then the shell would not be passing unescaped quotes through to the program. For example: $ echo "Hello, world!" Hello, world! Here echo only saw the unqouted string so that's what it displays. If you're not using a shell and starting this from another program then I'd assume there's some way to generate unquoted arguments in whatever you're using. > > 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. Yes, that's my expecation - it's just processing escape sequences and passing everything else through.
Ok, thanks for clarification. You can forget this patch! -----Original Message----- From: Mark Brown <broonie@kernel.org> Sent: Montag, 8. Februar 2021 11:09 To: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com> Cc: linux-spi@vger.kernel.org Subject: Re: [PATCH] spi: tools: fix input string formatting On Sat, Feb 06, 2021 at 10:57:04AM +0000, Aleksandar Gerasimovski wrote: > 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? If you don't want quotes to be sent then don't send them - my expectation would be that if you're driving this from the shell then the shell would not be passing unescaped quotes through to the program. For example: $ echo "Hello, world!" Hello, world! Here echo only saw the unqouted string so that's what it displays. If you're not using a shell and starting this from another program then I'd assume there's some way to generate unquoted arguments in whatever you're using. > > 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. Yes, that's my expecation - it's just processing escape sequences and passing everything else through.
diff --git a/tools/spi/spidev_test.c b/tools/spi/spidev_test.c index 83844f8..7c36677 100644 --- a/tools/spi/spidev_test.c +++ b/tools/spi/spidev_test.c @@ -89,7 +89,7 @@ static void hex_dump(const void *src, size_t length, size_t line_size, /* * Unescape - process hexadecimal escape character - * converts shell input "\x23" -> 0x23 + * converts shell input "\\x23" -> 0x23 */ static int unescape(char *_dst, char *_src, size_t len) { @@ -100,6 +100,10 @@ static int unescape(char *_dst, char *_src, size_t len) unsigned int ch; while (*src) { + if (*src == '"') { + src++; + continue; + } if (*src == '\\' && *(src+1) == 'x') { match = sscanf(src + 2, "%2x", &ch); if (!match) @@ -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++; } ret++;
The actual unescape implementation has two bugs: 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 2. there is not format check for decimal input strings First bug makes spidev_test unusable when strict spi sequence is needed, second bug is not nice to have it in. This patch improves unescape function and fixes above listed bugs. Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com> --- tools/spi/spidev_test.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)