Jeff King wrote:
> On Tue, Nov 15, 2011 at 11:22:48AM +0100, Thomas Rast wrote:
> >
> > I see valgrind failures bisecting to this commit, like so:
> >
> > ==19125== Syscall param execve(argv[i]) points to unaddressable byte(s)
> > ==19125== at 0x5303CB7: execve (in /lib64/libc-2.11.3.so)
> > ==19125== by 0x53045A5: execvpe (in /lib64/libc-2.11.3.so)
> > ==19125== by 0x4B183C: execv_git_cmd (exec_cmd.c:137)
> > ==19125== by 0x4F305E: start_command (run-command.c:277)
> > ==19125== by 0x47F5C9: cmd_upload_archive (upload-archive.c:98)
> > ==19125== by 0x4051F4: run_builtin (git.c:308)
> > ==19125== by 0x40538F: handle_internal_command (git.c:466)
> > ==19125== by 0x405556: main (git.c:553)
> > ==19125== Address 0x7feffe7d0 is not stack'd, malloc'd or (recently) free'd
> >
> > when running 'make valgrind' in current master. Let me know if you
> > need more information.
>
> With which test, and on what OS? I couldn't replicate running
> t5000 on Linux.
11, and many others after it, on a pretty vanilla opensuse 11.4 VM I
use almost exclusively for the valgrind runs. I used a pre-3.7 SVN
valgrind build, but 3.6.1 (shipped with opensuse) finds the same.
But after a closer look I think this patch just prodded it enough to
unearth long-existing undefined behaviour: prepare_argv() summarizes
to something like
static void prepare_argv(const char **sent_argv, const char **argv)
{
char *p, buf[4096];
for (p = buf;;) {
len = packet_read_line(0, p, (buf + sizeof buf) - p);
/* ... p always points into buf ... */
sent_argv[sent_argc++] = p;
p += len;
*p++ = 0;
}
sent_argv[sent_argc] = NULL;
}
The code appears to have looked like this ever since the addition of
that file back in 39345a2 (Add git-upload-archive, 2006-09-07). So
the elements of sent_argv have apparently always pointed into the
stack-allocated 'buf'.
(This correlates with the "Address 0x7feffe7d0 is not stack'd", even
though it's pretty clearly an address into the stack.)
A quick band-aid would be to heap-allocate it instead:
diff --git i/builtin/upload-archive.c w/builtin/upload-archive.c
index c57e8bd..6ab50d3 100644
--- i/builtin/upload-archive.c
+++ w/builtin/upload-archive.c
@@ -22,9 +22,10 @@ static const char lostchild[] =
static void prepare_argv(const char **sent_argv, const char **argv)
{
const char *arg_cmd = "argument ";
- char *p, buf[4096];
+ char *p, *buf;
int sent_argc;
int len;
+ buf = xmalloc(4096);
/* put received options in sent_argv[] */
sent_argc = 2;
@@ -32,7 +33,7 @@ static void prepare_argv(const char **sent_argv, const char **argv)
sent_argv[1] = "--remote-request";
for (p = buf;;) {
/* This will die if not enough free space in buf */
- len = packet_read_line(0, p, (buf + sizeof buf) - p);
+ len = packet_read_line(0, p, (buf + 4096) - p);
if (len == 0)
break; /* got a flush */
if (sent_argc > MAX_ARGS - 2)
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html