From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Tue, 15 Nov 2011 17:44:38 GMT
Subject: Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork
Message-ID: <CABPQNSbu9bb5UG1ihi2vGX5ZkpvjfjaXrT8bhDGZHb=n9RbjSg@mail.gmail.com>
In-Reply-To: <20111115173715.GA4478@sigill.intra.peff.net>

On Tue, Nov 15, 2011 at 6:37 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 15, 2011 at 01:11:46PM +0100, Thomas Rast wrote:
>
>> 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.
>

Outch. Thanks for spotting.

>> A quick band-aid would be to heap-allocate it instead:
>
> That works.  An even shorter band-aid is to mark it as "static".

Hmm, I seem to remember spotting it myself at some point and fixing it
by marking it as static. I guess I must have forgot to push it...

> I think the code would be more readable if it just used the new
> argv_array.
>

Oooh, nice. The whole argv_array slipped past me, I like it!