* Re: git segfault in tag verify (patch included)
2019-07-16 18:20 ` Junio C Hamano
@ 2019-07-16 18:47 ` Steven Roberts
2019-07-16 18:58 ` Jeff King
1 sibling, 0 replies; 5+ messages in thread
From: Steven Roberts @ 2019-07-16 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]
Thanks. I hope this works ok for you (see attached).
On Tue, Jul 16, 2019 at 11:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Steven Roberts <fenderq@gmail.com> writes:
>
> > I believe I have found an off-by-one error in git.
> >
> > Please see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2
>
> 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.
>
> We need your sign-off (see Documentation/SubmittingPatches).
>
> Thanks.
>
>
> -- >8 --
> From: Steven Roberts <fenderq@gmail.com>
> Subject: gpg-interface: do not scan past the end of buffer
>
> If the GPG output ends with trailing blank lines, after skipping
> them over inside the loop to find the terminating NUL at the end,
> the loop ends up looking for the next line, starting past the end.
>
> ---
> gpg-interface.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8ed274533f..eb55d46ea4 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -116,6 +116,9 @@ static void parse_gpg_output(struct signature_check *sigc)
> for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> while (*line == '\n')
> line++;
> + if (!*line)
> + break;
> +
> /* Skip lines that don't start with GNUPG status */
> if (!skip_prefix(line, "[GNUPG:] ", &line))
> continue;
>
--
Steven Roberts | https://www.fenderq.com/
[-- Attachment #2: 0001-gpg-interface-do-not-scan-past-the-end-of-buffer.patch --]
[-- Type: text/x-patch, Size: 837 bytes --]
From d48814273a50cf0b293148cc40a6a5cc7c13686e Mon Sep 17 00:00:00 2001
From: Steven Roberts <sroberts@fenderq.com>
Date: Tue, 16 Jul 2019 11:40:46 -0700
Subject: [PATCH] gpg-interface: do not scan past the end of buffer
Signed-off-by: Steven Roberts <sroberts@fenderq.com>
---
gpg-interface.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/gpg-interface.c b/gpg-interface.c
index 8ed274533f..775475131d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -116,6 +116,11 @@ static void parse_gpg_output(struct signature_check *sigc)
for (line = buf; *line; line = strchrnul(line+1, '\n')) {
while (*line == '\n')
line++;
+
+ /* Break out of trailing '\n' */
+ if (!*line)
+ break;
+
/* Skip lines that don't start with GNUPG status */
if (!skip_prefix(line, "[GNUPG:] ", &line))
continue;
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: git segfault in tag verify (patch included)
2019-07-16 18:20 ` Junio C Hamano
2019-07-16 18:47 ` Steven Roberts
@ 2019-07-16 18:58 ` Jeff King
2019-07-16 19:17 ` Junio C Hamano
1 sibling, 1 reply; 5+ messages in thread
From: Jeff King @ 2019-07-16 18:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steven Roberts, git
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
^ permalink raw reply [flat|nested] 5+ messages in thread