Another memory overrun in http-push.c

Another memory overrun in http-push.c

[Eygene Ryabinkin]

Me again ;))

Spotted another memory overrun in the http-push.c. In principle,
it is the read-only overrun, but it provokes the coredump on my
system.  The problem is that strlcpy(dst, src, size) returns the
length of the 'src' and demands it to be NULL-terminated (see
'man strlcpy' and http://www.gratisoft.us/todd/papers/strlcpy.html).
It is not the case for the xml_cdata and possibly other places. So
I've just replaced strlcpy with memcpy + zero termination all over
the http-push.c. The patch is below.

--- http-push.c.orig	Thu Mar  1 18:48:19 2007
+++ http-push.c	Thu Mar  1 18:55:24 2007
@@ -1271,7 +1271,9 @@
 	struct xml_ctx *ctx = (struct xml_ctx *)userData;
 	free(ctx->cdata);
 	ctx->cdata = xmalloc(len + 1);
-	strlcpy(ctx->cdata, s, len + 1);
+	/* NB: 's' is not null-terminated, can not use strlcpy here */
+	memcpy(ctx->cdata, s, len);
+	ctx->cdata[len] = '\0';
 }
 
 static struct remote_lock *lock_remote(const char *path, long timeout)
@@ -1473,7 +1475,8 @@
 		return;
 	path += 8;
 	obj_hex = xmalloc(strlen(path));
-	strlcpy(obj_hex, path, 3);
+	/* NB: path is not null-terminated, can not use strlcpy here */
+	memcpy(obj_hex, path, 2);
 	strcpy(obj_hex + 2, path + 3);
 	one_remote_object(obj_hex);
 	free(obj_hex);
@@ -2170,7 +2173,8 @@
 	/* If it's a symref, set the refname; otherwise try for a sha1 */
 	if (!strncmp((char *)buffer.buffer, "ref: ", 5)) {
 		*symref = xmalloc(buffer.posn - 5);
-		strlcpy(*symref, (char *)buffer.buffer + 5, buffer.posn - 5);
+		memcpy(*symref, (char *)buffer.buffer + 5, buffer.posn - 6);
+		(*symref)[buffer.posn - 6] = '\0';
 	} else {
 		get_sha1_hex(buffer.buffer, sha1);
 	}

memcpy(obj_hex, path, 2) is not followed by zero-termination since
it will be done by the strcpy that is following.

This cured my git-http-push and let it do all PROPFINDS on the rather
large repository (175 Mb). Now I have only one SEGV that is happening
inside the libcurl both in http-push.c and http-fetch.c. Already
talking to CURL people and trying to write the clear testcase for
the problem.
-- 
Eygene

Re: Another memory overrun in http-push.c

[Junio C Hamano]

Eygene Ryabinkin <rea-git@codelabs.ru> writes:

> Me again ;))
>
> Spotted another memory overrun in the http-push.c. In principle,
> it is the read-only overrun, but it provokes the coredump on my
> system.  The problem is that strlcpy(dst, src, size) returns the
> length of the 'src' and demands it to be NULL-terminated (see
> 'man strlcpy' and http://www.gratisoft.us/todd/papers/strlcpy.html).
> It is not the case for the xml_cdata and possibly other places. So
> I've just replaced strlcpy with memcpy + zero termination all over
> the http-push.c. The patch is below.

Please check Documentation/SubmittingPatches.

Use of strlcpy() in general _is_ stupid if you are computing how
much space is needed, allocating that much as your own buffer
and then copying.  strlcpy() needs to say how much it would have
copied if it were given large enough buffer, and it needs to be
able to run strlen(src), so it is not valid to give a buffer
that may not be NUL-terminated as you say.

Re: Another memory overrun in http-push.c

[Eygene Ryabinkin]

Junio, good day!

> > Spotted another memory overrun in the http-push.c. In principle,
> > it is the read-only overrun, but it provokes the coredump on my
> > system.  The problem is that strlcpy(dst, src, size) returns the
> > length of the 'src' and demands it to be NULL-terminated (see
> > 'man strlcpy' and http://www.gratisoft.us/todd/papers/strlcpy.html).
> > It is not the case for the xml_cdata and possibly other places. So
> > I've just replaced strlcpy with memcpy + zero termination all over
> > the http-push.c. The patch is below.
> 
> Please check Documentation/SubmittingPatches.

Thanks, just read and enlightened.

> Use of strlcpy() in general _is_ stupid if you are computing how
> much space is needed, allocating that much as your own buffer
> and then copying.  strlcpy() needs to say how much it would have
> copied if it were given large enough buffer, and it needs to be
> able to run strlen(src), so it is not valid to give a buffer
> that may not be NUL-terminated as you say.

Yes, you're perfectly right: the strncpy, bcopy or memcpy + zero-termination
are preferrable in this situation.
-- 
Eygene