Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

Am Wed, 11 Jul 2018 10:27:52 -0400
schrieb Jeff King [off-list ref]:

On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:

quoted

quoted

I think it's worth addressing in the near term, if only because
this kind of off-by-one is quite subtle, and I don't want to
forget to deal with it. Whether that happens as part of this
patch, or as a cleanup before or after, I'm not picky. :)  

I get that and if anyone is willing to write that code, i will base
my patches on it. What i want to avoid is taking responsibility for
problems i did not introduce, just because i happen to work on that
code at the moment. Keeping track of that (not forgetting) is also
not for the random contributor like myself.  

It doesn't make sense to do a patch before your series, since it would
just be:

  if (strlen(found) > 16)
    ...

Instead of randomly crashing on unexpected input, we would now silently
ignore it.

which would get obliterated by your patch. The patch after is shown
below. But frankly, it seems a lot easier to just handle this while
you are rewriting the code.

-- >8 --  
Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output

When parsing gpg's VALIDSIG lines, we look for a space
followed by the signer information. Because we use
strchrnul(), though, if the space is missing we'll end up
pointing to the trailing NUL. When we try to move past that
space, we have to handle the NUL case separately to avoid
accidentally stepping out of the string entirely.

True.

This would keep us in bounds of the unexpected string. But ignore the
line instead of "complaining" or crashing.

But you are right, it is easy enough and ignoring the line is probably
the best way of dealing with it.

i will change the condition to

if (*next && sigc-> result != 'E')

also skipping the following strchrnul and xmemdupz

Henning

 				next = strchrnul(found, '\n');
 				sigc->signer = xmemdupz(found, next
- found); }