Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()

From: NRK <nrk_AT_disroot.org>
Date: Wed, 17 Aug 2022 08:49:01 +0600

On Tue, Aug 16, 2022 at 08:58:37PM +0000, HushBugger wrote:
> Thanks, I don't have a lot of practical C experience.

The reason for the cast is because <ctype.h> is a poorly designed
library where the caller needs to ensure that the arg is representable
as an `unsigned char` (i.e 0 .. UCHAR_MAX) or as `EOF` [0].

         for (s = src, i = 0; *s; s++, i++) {
- if (*s == '%' && (sscanf(s + 1, "%2hhx", &n) == 1)) {
+ if (*s == '%' && isxdigit((unsigned char)s[1]) &&
+ isxdigit((unsigned char)s[2])) {
+ sscanf(s + 1, "%2hhx", &n);
                         dest[i] = n;
                         s += 2;

At a glance, the `s += 2` looks incorrect to me because it's reading 3
bytes, not 2. But then I saw that the for loop increments `s` as well.

IMO messing around with the loop counter in two places is not a good
idea because it makes the code harder to reason about than it needs to
be. I think the `s++` should be removed from the for loop and `s` should
be incremented as needed inside the loop instead.

- s += 2;
+ s += 3;
        } else {
- dest[i] = *s;
+ dest[i] = *s++;

But this is mostly style nit, so feel free to ignore.

[0]: https://port70.net/~nsz/c/c11/n1570.html#7.4

- NRK
Received on Wed Aug 17 2022 - 04:49:01 CEST

This archive was generated by hypermail 2.3.0 : Wed Aug 17 2022 - 04:48:55 CEST