Re: [PATCH] Improved error messages when temporary file creation fails

From: Arnout Engelen <arnouten@bzzt.net>
Date: Dec 7, 2010, 21:20 UTC
Message-ID: <20101207212041.GG25767@bzzt.net>
In-Reply-To: <7v62v5paj2.fsf@alter.siamese.dyndns.org>

Thanks to you and Jonathan again for the feedback.

On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:

> > +	char origtemplate[255];
> > +	strlcpy(origtemplate, template, 255);
> 
> Why "255"?

Random - 'i had to choose something'.

Show 5 quoted lines

> It may happen to be sufficiently large for the current callers, but what
> provisions if any are made to help the compiler or the runtime protect us
> from new and broken callers?  Use of strlcpy() there hides the issue from
> the runtime by avoiding segfault, but it actively harms us by making the
> code silently behave incorrectly without segfaulting, no?

Only in a small way: when a bigger template is encountered and the mkstemp call succeeds, there is no problem. Only when xmkstemp fails *and* clears the template, the diagnostic error message shows a truncated version of the original.

I *could* dynamically allocate space for the original template string, but that would mean I'd need to do a malloc() instead of putting the buffer on the stack like this, and free() it afterwards. I'm not too concerned about the performance hit here (presumably the I/O that comes with creating and using the temporary file here is orders of magnitude slower than that malloc() anyway), but it would also make the code a bit less easy to read.

What do you think would be preferable here, a simple fixed-length buffer on the stack that might cause a truncated error message or a dynamically-allocated one that makes the code somewhat more complicated?

> > +++ b/wrapper.h
> 
> Somewhat questionable...

Agreed, this whole file is unneeded and, well, wrong anyway.

I'll remove wrapper.h and apply Jonathan's improvements some time this week, unless of course someone beats me to it :).

Kind regards,

Arnout