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

[Jeff King <peff@peff.net>]

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

> 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