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

--

[ message continues ]

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

--

[ message continues ]

Yup, it would have caught it, thanks.

Acked-by: Shawn O. Pearce <spearce@spearce.org>
 
-- 
Shawn.
--

[ message continues ]

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.
--

[ message continues ]

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
--

[ message continues ]

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';
--

[ message continues ]

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
--

[ message continues ]

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--

[ message continues ]