svn commit: r309364 - head/usr.bin/locale

[Bruce Evans]

On Thu, 1 Dec 2016, Eric van Gyzen wrote:

> Log:
>  locale: fix buffer management
>
>  Also, handle signed and unsigned chars, and more gracefully handle
>  invalid input.
>
>  Submitted by:	bde in response to r309331
>  MFC after:	1 week
>  Sponsored by:	Dell EMC

Thanks.

> Modified: head/usr.bin/locale/locale.c
> ==============================================================================
> --- head/usr.bin/locale/locale.c	Thu Dec  1 15:46:26 2016	(r309363)
> +++ head/usr.bin/locale/locale.c	Thu Dec  1 16:54:02 2016	(r309364)
> @@ -495,29 +495,29 @@ format_grouping(const char *binary)
> {
> 	static char rval[64];
> 	const char *cp;
> -	size_t len;
> +	size_t roff;
> +	int len;
> ...
> +		if (*cp < 0)
> +			break;		/* garbage input */
> +		len = snprintf(&rval[roff], sizeof(rval) - roff, "%u;", *cp);
> +		if (len < 0 || (unsigned)len >= sizeof(rval) - roff)
> +			break;		/* insufficient space for output */

I don't like casting len to fix compiler warnings, and intentionally
left out this cast.  len < 0 ensures that there is no sign extension bug
or dependency on sign extension bugs.  snprintf() returns < 0 if an
encoding error occurs, and the check is mainly to detect this unlikely
error (the comment is too short to describe this), but the check also
allows compilers to easily see that there is no sign extension bug and
only low-quality ones warn.

I used the bogus type size_t instead of int for roff since I didn't
want to fight possible compiler warnings about sign extension in the
expression sizeof(rval) - roff.  Here it is not so easy to see that
roff <= sizeof(rval).

You didn't do anything to avoid bad -Wtautological-compare compiler
warnings for (*cp < 0).  I think such warnings occur at high WARNS
on arches with unsigned char.  These warnings are bad since the
comparison is only tautological on some arches so it is not really
tautological.  Avoiding it requires code like '#if CHAR_MIN < 0', but
that is equally tautological and only escapes the warning because cpp
expressions are weaker than C expressions.

clang -Wall -Wtautological-compare doesn't actually warn for either
(*cp < 0) (where cp is unsigned char *) or (1 < 0).  gcc-4.2.1 -Wall
warns for the former but not for the latter.  This seems sort of
backwards, but ISTR discussions in gcc lists ~20 years ago about it
being intentional.  Warning for (1 < 0) mainly breaks conditional
compilation where the condtions are intentially written using C
const expressions instead of cpp expressions since cpp is harder to
work with.  (*cp < 0) is more likely to be a bug.

Bruce