Re: [PATCH] system_path: use a static buffer

From: Jeff King <peff@peff.net>
Date: Mar 21, 2011, 15:51 UTC
Message-ID: <20110321155108.GA27662@sigill.intra.peff.net>
In-Reply-To: <1300721194.2583.22.camel@bee.lab.cmartin.tk>

On Mon, Mar 21, 2011 at 04:26:29PM +0100, Carlos Martín Nieto wrote:

Show 9 quoted lines

> > FWIW, we don't catch snprintf failures in 99% of the calls in git. Most
> > calls just ignore the return value, and some even directly use the
> > return value to add to a length. The one place that actually does check
> > for the error is strbuf_vaddf, which just says "your vsnprintf is
> > broken" and dies.
> 
>  It's not actually likely we'll ever meet this error if the only one
> allowed to set the format string is the programmer (and to do otherwise
> is a security risk).

Agreed.

Show 6 quoted lines

>  Or we could overload (#define) snprintf and replace it with the
> paranoid. It'd go nicely with the vsnprintf that tries to work around
> the Windows implementation.
> 
>  I don't feel that strongly we should have the extra check there, seeing
> how it's rare and not checked anywhere else.

Yeah, I am happy to just drop it. AFAICT, an error return from snprintf is a bug in your program[1] or a bug in libc. In either case, it should fail or produce bogus output on the first run, and we don't need to worry about checking errors all the time. Noting the error helps a little with detection, but since we already install our own vsnprintf on known-buggy platforms, I haven't seen a single complaint.

-Peff

[1] The only error I managed to get out of eglibc was trying to format "%" (i.e., percent followed by NUL). Skimming the eglibc code (gaah, my eyes!) it looks like under some conditions it can allocate small work buffers, and return an error if malloc fails. So yeah, I guess it can fail randomly, but it seems pretty unlikely.