Re: [Xen-devel] [PATCH] xentrace: build fix

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Jan Beulich writes ("Re: [Xen-devel] [PATCH] xentrace: build fix"):
> >>> On 11.01.11 at 15:53, Christoph Egger <Christoph.Egger@xxxxxxx> wrote:
> > Attached patch fixes warnings:
> > "array subscript has type 'char'"
> 
> May I ask with what kind of broken ctype.h you encountered this? It
> should be very natural to pass a plain 'char' to the is...() functions,
> and it looks rather broken to have these casts in there.

I'm afraid you're wrong.  You are right that it "_should_ be very
natural", but "should be " is not the same as "is" and in fact passing
an arbitrary char to isfoobar() can have undefined behaviour.

Here is the SuS page for isspace:
   http://pubs.opengroup.org/onlinepubs/9699919799/functions/isspace.html

It says:
   The c argument is an int, the value of which the application shall
   ensure is a character representable as an unsigned char or equal to
   the value of the macro EOF. If the argument has any other value,
   the behavior is undefined.

This a convoluted way of saying that passing a negative value other
than EOF to isfoobar is allowed to crash.  That can happen if char is
signed (as it usually is) whenever a high-bit-set character (nowadays,
utf-8) is found.


This means that the natural and "obviously correct" use of the ctype
macros,
   char c = some_unknown_string[n];
   if (isspace(c)) { ...
is wrong.

The correct usage (assuming c cannot possibly be EOF) is:
   if (isspace((unsigned char)c)) { ...

Yes, this is horrid but it is a requirement of the specification.

Many projects have a standard CTYPE macro which allows you to write
   if (CTYPE(isspace,c)) { ...

Xen (particularly the external parts of stubdom) is absolutely full of
bugs of this kind but most of them aren't all that problematic because
the characters in question come from trusted sources like the Xen
command line or domain config file or whatever.

My grep found this:
  xen/include/acpi/platform/acenv.h:#define ACPI_IS_SPACE(i)        
isspace((int) (i))
which is a way of disabling the warning while continuing to have the
bug.


NB, Christoph, the correct usage does not generally involve uint8_t
(although in a Xen context where unsigned char is guaranteed to be the
same size as uint8_t we don't care that much).


Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel