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

From: Thomas Rast <trast@student.ethz.ch>
Date: Jun 12, 2009, 10:04 UTC
Message-ID: <200906121204.37752.trast@student.ethz.ch>
In-Reply-To: <7v1vppbyud.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 5 quoted lines

> * 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:

Show 13 quoted lines

> @@ -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 '%'.

Show 6 quoted lines

> +	}
> +	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