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@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html