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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2010, 20:56 UTC
Message-ID: <7v62v5paj2.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20101207181633.GF25767@bzzt.net>

Arnout Engelen <arnouten@bzzt.net> writes:

> This patch has been submitted/discussed before, but that version doesn't apply
> cleanly to the newest git anymore, so here's an updated version.

Thanks, but that comes after "---", no?

> It improves diagnostic error messages when creating a temporary file fails.
>
> Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
> ---

Show 18 quoted lines

> diff --git a/wrapper.c b/wrapper.c
> index 4c1639f..6640c87 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -2,6 +2,7 @@
>   * Various trivial helper wrappers around standard functions
>   */
>  #include "cache.h"
> +#include "wrapper.h"
>  
>  static void do_nothing(size_t size)
>  {
> @@ -196,10 +197,20 @@ FILE *xfdopen(int fd, const char *mode)
>  int xmkstemp(char *template)
>  {
>  	int fd;
> +	char origtemplate[255];
> +	strlcpy(origtemplate, template, 255);

Why "255"?

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?

Show 10 quoted lines

> diff --git a/wrapper.h b/wrapper.h
> new file mode 100644
> index 0000000..b06ff0d
> --- /dev/null
> +++ b/wrapper.h
> @@ -0,0 +1,4 @@
> +/*
> + * Various trivial helper wrappers around standard functions
> + */
> +int xmkstemp(char *template);

Somewhat questionable...

Why doesn't this say "extern"? What happend to another copy of this line in git-compat-util.h? IOW, what protects this and the other one from drifting apart? Why doesn't users include wrapper.h but only wrapper.c? Why yet another header is needed only for this function and nothing else? Why isn't this protected with the standard #ifndef .../#define .../#endif?