Re: What's cooking in git.git (Jun 2009, #01; Fri, 12)

[Thomas Rast <trast@student.ethz.ch>]

Junio C Hamano wrote:
> * tr/die_errno (Mon Jun 8 23:02:20 2009 +0200) 4 commits
>  - Use die_errno() instead of die() when checking syscalls
>  - Convert existing die(..., strerror(errno)) to die_errno()
>  - die_errno(): double % in strerror() output just in case
>  - Introduce die_errno() that appends strerror(errno) to die()

I had a look at your

  [526abe63] die_errno(): double % in strerror() output just in case

and it seems it doesn't cover all corner cases:

> @@ -64,8 +64,20 @@ void die_errno(const char *fmt, ...)
>  {
>  	va_list params;
>  	char fmt_with_err[1024];
> -
> -	snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, strerror(errno));
> +	char str_error[256], *err;
> +	int i, j;
> +
> +	err = strerror(errno);
> +	for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {
> +		if ((str_error[j++] = err[i++]) != '%')
> +			continue;

If we copied a '%' here, but filled 'str_error', then

> +		if (j < sizeof(str_error) - 1)
> +			str_error[j++] = '%';
> +		else
> +			break;

we 'break' here, instead of tacking on another '%'.  This subsequently
leaves a single trailing '%' at the end of the string.  A possible fix
would be to use

+		else {
+			j--;
+			break;
+		}

instead, so that the terminator ends up on the second-to-last position
in the string, overwriting the lonely '%'.

> +	}
> +	str_error[j] = 0;
> +	snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, str_error);
>  
>  	va_start(params, fmt);
>  	die_routine(fmt_with_err, params);


I cannot find an explicit mention that trailing '%'s are bad, and my
printf() just ignores them, but 'man 3p printf' (showing a "POSIX
Programmer's Manual" entry that I can't seem to find on the web...) at
least states

  [after explaining all conversion specifiers]

  If a conversion specification does not match one of the above forms,
  the behavior is undefined. If any argument is not the correct type
  for the corresponding conversion specification, the behavior is
  undefined.


[I still like v2 better because it's far less complicated...]

-- 
Thomas Rast
trast@{inf,student}.ethz.ch