* [PATCH 1/2] http-backend: Fix access beyond end of string. @ 2009-11-14 21:10 Tarmigan Casebolt 2009-11-14 21:10 ` [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions Tarmigan Casebolt 2009-11-16 1:36 ` [PATCH 1/2] http-backend: Fix access beyond end of string Shawn O. Pearce 0 siblings, 2 replies; 8+ messages in thread From: Tarmigan Casebolt @ 2009-11-14 21:10 UTC (permalink / raw) To: Shawn O . Pearce, git; +Cc: Tarmigan Casebolt Found with valgrind while looking for Content-Length corruption in smart http. Signed-off-by: Tarmigan Casebolt <tarmigan+git@gmail.com> --- http-backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/http-backend.c b/http-backend.c index f8ea9d7..ab9433d 100644 --- a/http-backend.c +++ b/http-backend.c @@ -634,7 +634,7 @@ int main(int argc, char **argv) cmd = c; cmd_arg = xmalloc(n); strncpy(cmd_arg, dir + out[0].rm_so + 1, n); - cmd_arg[n] = '\0'; + cmd_arg[n-1] = '\0'; dir[out[0].rm_so] = 0; break; } -- 1.6.5.51.g191f5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions. 2009-11-14 21:10 [PATCH 1/2] http-backend: Fix access beyond end of string Tarmigan Casebolt @ 2009-11-14 21:10 ` Tarmigan Casebolt 2009-11-16 1:39 ` Shawn O. Pearce 2009-11-16 1:36 ` [PATCH 1/2] http-backend: Fix access beyond end of string Shawn O. Pearce 1 sibling, 1 reply; 8+ messages in thread From: Tarmigan Casebolt @ 2009-11-14 21:10 UTC (permalink / raw) To: Shawn O . Pearce, git; +Cc: Tarmigan Casebolt We already have these checks in many printf-type functions that have prototypes which are in header files. Add these same checks to static functions in http-backend.c Signed-off-by: Tarmigan Casebolt <tarmigan+git@gmail.com> --- Shawn, please consider this patch in addition to the one that you posted that actually fixes the bug. With this patch, gcc will warn about that bug. http-backend.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/http-backend.c b/http-backend.c index ab9433d..110b166 100644 --- a/http-backend.c +++ b/http-backend.c @@ -108,6 +108,7 @@ static const char *get_parameter(const char *name) return i ? i->util : NULL; } +__attribute__((format (printf, 2, 3))) static void format_write(int fd, const char *fmt, ...) { static char buffer[1024]; @@ -165,6 +166,7 @@ static void end_headers(void) safe_write(1, "\r\n", 2); } +__attribute__((format (printf, 1, 2))) static NORETURN void not_found(const char *err, ...) { va_list params; @@ -180,6 +182,7 @@ static NORETURN void not_found(const char *err, ...) exit(0); } +__attribute__((format (printf, 1, 2))) static NORETURN void forbidden(const char *err, ...) { va_list params; -- 1.6.5.51.g191f5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions. 2009-11-14 21:10 ` [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions Tarmigan Casebolt @ 2009-11-16 1:39 ` Shawn O. Pearce 0 siblings, 0 replies; 8+ messages in thread From: Shawn O. Pearce @ 2009-11-16 1:39 UTC (permalink / raw) To: Tarmigan Casebolt; +Cc: git Tarmigan Casebolt <tarmigan+git@gmail.com> wrote: > We already have these checks in many printf-type functions that have > prototypes which are in header files. Add these same checks to > static functions in http-backend.c > > Signed-off-by: Tarmigan Casebolt <tarmigan+git@gmail.com> > --- > > Shawn, please consider this patch in addition to the one that you posted > that actually fixes the bug. With this patch, gcc will warn about that bug. Yup, it would have caught it, thanks. Acked-by: Shawn O. Pearce <spearce@spearce.org> -- Shawn. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] http-backend: Fix access beyond end of string. 2009-11-14 21:10 [PATCH 1/2] http-backend: Fix access beyond end of string Tarmigan Casebolt 2009-11-14 21:10 ` [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions Tarmigan Casebolt @ 2009-11-16 1:36 ` Shawn O. Pearce 2009-11-16 4:55 ` Jeff King 1 sibling, 1 reply; 8+ messages in thread From: Shawn O. Pearce @ 2009-11-16 1:36 UTC (permalink / raw) To: Tarmigan Casebolt, Junio C Hamano; +Cc: git Tarmigan Casebolt <tarmigan+git@gmail.com> wrote: > diff --git a/http-backend.c b/http-backend.c > index f8ea9d7..ab9433d 100644 > --- a/http-backend.c > +++ b/http-backend.c > @@ -634,7 +634,7 @@ int main(int argc, char **argv) > cmd = c; > cmd_arg = xmalloc(n); > strncpy(cmd_arg, dir + out[0].rm_so + 1, n); > - cmd_arg[n] = '\0'; > + cmd_arg[n-1] = '\0'; > dir[out[0].rm_so] = 0; > break; Shouldn't this instead be: diff --git a/http-backend.c b/http-backend.c index 9021266..16ec635 100644 --- a/http-backend.c +++ b/http-backend.c @@ -626,7 +626,7 @@ int main(int argc, char **argv) } cmd = c; - cmd_arg = xmalloc(n); + cmd_arg = xmalloc(n + 1); strncpy(cmd_arg, dir + out[0].rm_so + 1, n); cmd_arg[n] = '\0'; dir[out[0].rm_so] = 0; The cmd_arg string was simply allocated too small. Your fix is terminating the string one character too short which would cause get_loose_object and get_pack_file to break. -- Shawn. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] http-backend: Fix access beyond end of string. 2009-11-16 1:36 ` [PATCH 1/2] http-backend: Fix access beyond end of string Shawn O. Pearce @ 2009-11-16 4:55 ` Jeff King 2009-11-16 6:12 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Jeff King @ 2009-11-16 4:55 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Tarmigan Casebolt, Junio C Hamano, git On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote: > Tarmigan Casebolt <tarmigan+git@gmail.com> wrote: > > diff --git a/http-backend.c b/http-backend.c > > index f8ea9d7..ab9433d 100644 > > --- a/http-backend.c > > +++ b/http-backend.c > > @@ -634,7 +634,7 @@ int main(int argc, char **argv) > > cmd = c; > > cmd_arg = xmalloc(n); > > strncpy(cmd_arg, dir + out[0].rm_so + 1, n); > > - cmd_arg[n] = '\0'; > > + cmd_arg[n-1] = '\0'; > > dir[out[0].rm_so] = 0; > > break; > > Shouldn't this instead be: > > diff --git a/http-backend.c b/http-backend.c > index 9021266..16ec635 100644 > --- a/http-backend.c > +++ b/http-backend.c > @@ -626,7 +626,7 @@ int main(int argc, char **argv) > } > > cmd = c; > - cmd_arg = xmalloc(n); > + cmd_arg = xmalloc(n + 1); > strncpy(cmd_arg, dir + out[0].rm_so + 1, n); > cmd_arg[n] = '\0'; > dir[out[0].rm_so] = 0; > > The cmd_arg string was simply allocated too small. Your fix is > terminating the string one character too short which would cause > get_loose_object and get_pack_file to break. Actually, from my reading, I think his fix is right, because you trim the first character during the strncpy (using "out[0].rm_so + 1"). But it's not clear when you create 'n' that you are dropping that character. IOW, you are doing: /* string + '\0' - '/' */ size_t n = out[0].rm_eo - (out[0].rm_so + 1) + 1; which ends up the same as your n, but means that the NUL goes at cmd_arg[n-1]. But I didn't actually run it, so if his fix is breaking things, then both Tarmigan and I are counting wrong. ;) -Peff ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] http-backend: Fix access beyond end of string. 2009-11-16 4:55 ` Jeff King @ 2009-11-16 6:12 ` Junio C Hamano 2009-11-17 4:46 ` Tarmigan 2009-11-23 17:20 ` Brian Gernhardt 0 siblings, 2 replies; 8+ messages in thread From: Junio C Hamano @ 2009-11-16 6:12 UTC (permalink / raw) To: Jeff King; +Cc: Shawn O. Pearce, Tarmigan Casebolt, Junio C Hamano, git Jeff King <peff@peff.net> writes: > On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote: > ... >> Shouldn't this instead be: >> >> diff --git a/http-backend.c b/http-backend.c >> index 9021266..16ec635 100644 >> --- a/http-backend.c >> +++ b/http-backend.c >> @@ -626,7 +626,7 @@ int main(int argc, char **argv) >> } >> >> cmd = c; >> - cmd_arg = xmalloc(n); >> + cmd_arg = xmalloc(n + 1); >> strncpy(cmd_arg, dir + out[0].rm_so + 1, n); >> cmd_arg[n] = '\0'; >> dir[out[0].rm_so] = 0; >> >> The cmd_arg string was simply allocated too small. Your fix is >> terminating the string one character too short which would cause >> get_loose_object and get_pack_file to break. > > Actually, from my reading, I think his fix is right, because you trim > the first character during the strncpy (using "out[0].rm_so + 1"). Your regexps all start with leading "/", and rm_so+1 points at the character after the slash; the intention being that you would copy the rest of the matched sequence without the leading "/". So allocating n = rm_eo - rm_so is Ok. It counts the space for terminating NUL. But copying "up to n bytes" using strncpy(), only to NUL terminate immediately later, is dubious. You would want to copy only n-1 bytes. I.e. n = out[0].rm_eo - out[0].rm_so; /* allocation */ ... validate and fail invalid method ... cmd_arg = xmalloc(n); memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1); cmd_arg[n-1] = '\0'; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] http-backend: Fix access beyond end of string. 2009-11-16 6:12 ` Junio C Hamano @ 2009-11-17 4:46 ` Tarmigan 2009-11-23 17:20 ` Brian Gernhardt 1 sibling, 0 replies; 8+ messages in thread From: Tarmigan @ 2009-11-17 4:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Shawn O. Pearce, git On Sun, Nov 15, 2009 at 10:12 PM, Junio C Hamano <gitster@pobox.com> wrote: > Jeff King <peff@peff.net> writes: > >> On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote: >> ... >>> Shouldn't this instead be: >>> >>> diff --git a/http-backend.c b/http-backend.c >>> index 9021266..16ec635 100644 >>> --- a/http-backend.c >>> +++ b/http-backend.c >>> @@ -626,7 +626,7 @@ int main(int argc, char **argv) >>> } >>> >>> cmd = c; >>> - cmd_arg = xmalloc(n); >>> + cmd_arg = xmalloc(n + 1); >>> strncpy(cmd_arg, dir + out[0].rm_so + 1, n); >>> cmd_arg[n] = '\0'; >>> dir[out[0].rm_so] = 0; >>> >>> The cmd_arg string was simply allocated too small. Your fix is >>> terminating the string one character too short which would cause >>> get_loose_object and get_pack_file to break. >> >> Actually, from my reading, I think his fix is right, because you trim >> the first character during the strncpy (using "out[0].rm_so + 1"). > > Your regexps all start with leading "/", and rm_so+1 points at the > character after the slash; the intention being that you would copy > the rest of the matched sequence without the leading "/". > > So allocating n = rm_eo - rm_so is Ok. It counts the space for > terminating NUL. But copying "up to n bytes" using strncpy(), only to NUL > terminate immediately later, is dubious. You would want to copy only n-1 > bytes. I.e. > > n = out[0].rm_eo - out[0].rm_so; /* allocation */ > ... validate and fail invalid method ... > cmd_arg = xmalloc(n); > memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1); > cmd_arg[n-1] = '\0'; > I think the strncpy( , ,n) would not harm anything because we won't overflow dir because it's NUL terminated in getdir(), and the '\0' shouldn't match the regex. But I agree that strncpy( , , n-1) is better and memcpy( , , n-1) is better still. Better eyes than mine have now looked at this and see different things each time. I wonder if some parts could be made a little less subtle (perhaps along with the dir[out[0].rm_so] = 0;)? Thanks, Tarmigan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] http-backend: Fix access beyond end of string. 2009-11-16 6:12 ` Junio C Hamano 2009-11-17 4:46 ` Tarmigan @ 2009-11-23 17:20 ` Brian Gernhardt 1 sibling, 0 replies; 8+ messages in thread From: Brian Gernhardt @ 2009-11-23 17:20 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Shawn O. Pearce, Tarmigan Casebolt, Git List, Tay Ray Chuan On Nov 16, 2009, at 1:12 AM, Junio C Hamano wrote: > n = out[0].rm_eo - out[0].rm_so; /* allocation */ > ... validate and fail invalid method ... > cmd_arg = xmalloc(n); > memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1); > cmd_arg[n-1] = '\0'; I just thought I'd point out that this change (committed as 48aec1b) fixed the problem I was having with t5541-http-push (and a couple others) hanging. Looks like that one extra byte was overwriting something that malloc/free wanted to keep intact on OS X. ~~ Brian ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-11-23 17:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-11-14 21:10 [PATCH 1/2] http-backend: Fix access beyond end of string Tarmigan Casebolt 2009-11-14 21:10 ` [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions Tarmigan Casebolt 2009-11-16 1:39 ` Shawn O. Pearce 2009-11-16 1:36 ` [PATCH 1/2] http-backend: Fix access beyond end of string Shawn O. Pearce 2009-11-16 4:55 ` Jeff King 2009-11-16 6:12 ` Junio C Hamano 2009-11-17 4:46 ` Tarmigan 2009-11-23 17:20 ` Brian Gernhardt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.