On Tue, Jul 16, 2019 at 11:20:48AM -0700, Junio C Hamano wrote:
> That is this thing.
>
> static void parse_gpg_output(struct signature_check *sigc)
> {
> const char *buf = sigc->gpg_status;
> const char *line, *next;
> int i, j;
> int seen_exclusive_status = 0;
>
> /* Iterate over all lines */
> for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> while (*line == '\n')
> line++;
> /* Skip lines that don't start with GNUPG status */
> if (!skip_prefix(line, "[GNUPG:] ", &line))
> continue;
>
> If the GPG output ends with a trailing blank line, we skip and get
> to the terminating NUL, then find that it does not begin with
> the "[GNUPG:] " prefix, and hit the continue. We try to scan and
> look for LF (or stop at the end of the string) for the next round,
> starting at one past where we are, which is already the terminating
> NUL. Ouch.
>
> Good finding.
Yeah. The patch below looks fine, and I do not see any other
out-of-bounds issues in the code (there is a similar "next + 1" in the
inner loop, but it checks for an empty line right beforehand already).
I find these kind of "+1" pointer increments without constraint checking
are error-prone. I suspect this whole loop could be a bit easier to
follow by finding the next line boundary at the start of the loop, and
jumping to it in the for-loop advancement. And then within the loop, we
know the boundaries of the line (right now, for example, we issue a
strchrnul() looking for a space that might unexpectedly cross line
boundaries).
Something like:
for (line = buf; *line; line = next) {
next = strchrnul(line, '\n');
... do stuff ...
/* find a space within the line */
space = memchr(line, ' ', next - line);
}
or even:
for (line = buf; *line; line += len) {
size_t len = strchrnul(line, '\n') - line;
...
space = memchr(line, ' ', linelen);
}
but it may not be worth the churn. It's just as likely to introduce a
new bug. ;)
I've also found working with strbuf_getline() to be very pleasant for a
lot of these cases (in which you get a real per-line NUL-terminated
string), but of course it implies an extra copy.
-Peff
