Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork
- From
Jeff King <peff@peff.net>
- Date
- Nov 15, 2011, 17:37 UTC
- Message-ID
- <20111115173715.GA4478@sigill.intra.peff.net>
- In-Reply-To
- <201111151311.46832.trast@student.ethz.ch>
On Tue, Nov 15, 2011 at 01:11:46PM +0100, Thomas Rast wrote:
Show 22 quoted lines
> 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'.Oh, yikes. That is definitely the problem, but it does come from c09cd77e. The prepare_argv function used to be "run_upload_archive", and it would prepare argv on the stack, call into write_archive with it, and then return; nobody else cares about the value afterwards.
Erik's patch converts it into a function that writes the new argv into a parameter and returns, and the now-invalid stack-allocated memory is used by the calling function.
> A quick band-aid would be to heap-allocate it instead:
That works. An even shorter band-aid is to mark it as "static".
I think the code would be more readable if it just used the new argv_array.
Junio, this bug is in 1.7.8-rc*. Do you want the one-liner fix for the release, or the nicer fix?
-Peff