Re: [PATCH] system_path: use a static buffer

["Carlos Martín Nieto" <cmn@elego.de>]

On lun, 2011-03-21 at 07:14 -0400, Jeff King wrote:
> On Mon, Mar 21, 2011 at 10:56:19AM +0100, Carlos Martín Nieto wrote:
> 
> > On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:
> > > Carlos Martín Nieto <cmn@elego.de> writes:
> > > 
> > > > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > > > +	if (ret >= sizeof(buf))
> > > > +		die("system path too long for %s", path);
> > > > +	else if (ret < 0)
> > > > +		die_errno("encoding error");
> > > 
> > > POSIX says snprintf() should set errno in this case, and your use of
> > > die_errno() would show that information, but what is "encoding error"?
> > > 
> > > Just being curious, as I suspect that "snprintf() returned an error" may
> > > be more appropriate, if the answer is "I don't know what kind of error it
> > > is, but snprintf() found something faulty while encoding so I chose to
> > > call it encoding error".
> > 
> >  My manpage says snprintf returns -1 if there was an output or encoding
> > error. As there couldn't be an output error because it's writing to
> > memory and we can't output what snprintf chocked on because whatever
> > die_errno uses will also choke on it, I just put "encoding error". I'd
> > put "error assembling system path" as the actual error message, I guess.
> 
> 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).

> 
> So I'm not sure how much we really care about this error code path. If
> anything, we should be replacing all of the calls with something like:
> 
>   static const char buggy_sprintf_msg[] =
>   "BUG: vsnprintf returned %d; either we fed it a bogus format string\n"
>   "(our bug) or your libc is buggy and returns an error when it should\n"
>   "tell us how much space is needed. The format string was:\n"
>   "%s\n";
>   int xsnprintf(char *out, size_t size, const char *fmt, ...)
>   {
>           va_list ap;
>           int r;
> 
>           va_start(ap, fmt);
>           r = vsnprintf(out, size, fmt, ap);
>           va_end(ap);
> 
>           if (r < 0)
>                   die(buggy_sprintf_msg, r, fmt);
>           return r;
>   }

 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.

   cmn