Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

From: Jeff King <peff@peff.net>
Date: Jun 11, 2010, 05:12 UTC
Message-ID: <20100611051236.GA3947@coredump.intra.peff.net>
In-Reply-To: <AANLkTimEwV_bJkd_2csJB0L6T9Lq6F0hpllUO2pJTL8m@mail.gmail.com>

On Thu, Jun 10, 2010 at 08:09:36PM -0300, Erick Mattos wrote:

Show 15 quoted lines

> 2010/6/10 Thomas Rast <trast@student.ethz.ch>:
> > What the - side of the hunk above does is returning a local (stack
> > allocated) variable, in the form of a pointer to logfile.  Once those
> > go out of scope, you have zero guarantees on what happens with them.
> 
> Not really.
> 
> What the actual log_ref_setup() does when is instantiated is to create
> a pointer in the stack, called log_file, to a pointer to a char array.
>  This pointer receives the address of a char array of the calling
> function because that is why passing by reference is made to.  See
> that the calling functions is using the "&" when making the call (If I
> was using C++ I would pass by reference the array itself but in C I
> can only pass pointer variables by reference that is why the pointer
> to a pointer).

No, Thomas is right. This invokes undefined behavior. We point the passed-in log_file pointer to the front of a character array with automatic duration. After log_ref_setup returns, we must never dereference that pointer again, but we do. So we need this patch or something like it.

In practice, it worked because allocating on the stack is really just about bumping the stack pointer, so that memory sits there until another function call needs it for stack variables. After returning from log_ref_setup, we don't actually make any other function calls before calling open(log_file), so the buffer was still there, untouched. There is a later use of log_file which is probably bogus, but was likely never triggered because it is in an unlikely error conditional.

> Then git_snpath() creates a char array in the heap with the right
> content and changes the stack pointer logfile to it.  Then when we do

No, it doesn't. git_snpath writes into the buffer you provide it, just like snprintf (hence the name).

> > Admittedly my experience is somewhat limited since I don't do C coding
> > outside of git and some teaching.  But so far I have not had a single
> > false alarm with valgrind (when compiled without optimizations;
> > otherwise the compiler may do some magic).

We have some false positives in git, but you don't see them because t/valgrind/default.supp suppresses them. For example:

  http://thread.gmane.org/gmane.comp.version-control.git/106335/focus=107302

If you are using a binary package of valgrind, it probably ships with some system-specific suppressions, too. Right now valgrind on Debian unstable is next to useless because glibc has been upgraded to 2.11, but the suppressions haven't been updated. So you get false positives all over the place because of clever architecture-specific optimizations (e.g., I am seeing a lot of __strlen_sse2 problems, which are probably just the function over-reading its input data because processing big chunks is faster).

-Peff