[PATCH 1/2] http-backend: Fix access beyond end of string.

Found with valgrind while looking for Content-Length corruption in
smart http.

Signed-off-by: Tarmigan Casebolt <redacted>
---
 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

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

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

Tarmigan Casebolt [off-list ref] 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.

-- 
Shawn.

Tarmigan Casebolt [off-list ref] 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 <redacted>
---

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

On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:

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