Re: array bounds, sanitizer, safe programming, and cilk array notation

[Martin Uecker <uecker at eecs dot berkeley dot edu>]

Marek Polacek <polacek@redhat.com>:

> Sorry for late reply - I've found this in my inbox only today.
> 
> On Mon, Jan 26, 2015 at 11:53:59AM -0800, Martin Uecker wrote:
>
> > Finally, what is missing is a way to diagnose problems inside
> > the called functions. -Warray-bounds=2 (with my recently
> > accepted patch) helps with this, but - so far - only for static 
> > arrays:
> > 
> > void foo(int (*x)[4])
> > {
> > 	(*x)[4] = 5;	// warning
> > }
>  
> This is detected by -fsanitize=object-size, turned on by default in
> -fsanitize=undefined.  Since it makes use of __builtin_object_size,
> it is necessary to compile with optimizations turned on.

Not always.  -fsanitize=object-size  does not seem to detect the
illegal out-of-bound access by itself, but instead uses some very 
coarse notion of object size. For example, the following was not
detected in my tests:


void foo(int (*f)[4])
{
        (*f)[4] = 1;	// warning
}

void bar(int n, int (*f)[n])
{
        (*f)[n] = 1;	// no warning
}

...
struct { int buf[4]; int x; } s;
foo(&s.buf);
bar(4, &s.buf);



Also in cases like this a compile-time warning/error is possible.
A compile-time warning is more useful, and I think -fsanitize should
not be a replacement for proper compile time warnings/errors.

Instead, I think it should catch exactly those cases, which cannot
be detected a compile time (i.e. VLA with size not known at compile
time) - so that the combination of -fsanitize with compile time errors
gives strict checking without unnecessary runtime checks.

... 

> > The later does not currently produce a warning, because x
> > is converted to a pointer and the length is ignored. 
> > 
> > If it is not possible to have warning here for compatibility
> > reasons, one possibility is to have an extension similar to 
> > 'static' which makes 'x' a proper array in the callee, e.g. 
> 
> I think even the 'static in parameter array declarators' (ugly) C99 extension
> isn't properly implemented yet (I don't think that the compiler makes
> any optimization based on it).

This is correct, it doesn't do anything now.

(I am still puzzled why the C committee did not just make the
parameter have a proper array type whenever a size is specified.
So would have been so much better and is what most programmers
would expect.)

> So - if I understood this correctly - I think it's better to enhance
> ubsan than to add any kind of language extensions.

ubsan should be enhanced to detect the cases which cannot
be detected at compile time. But it needs be able to detect the
out-of-bounds access directly, and not just access outside
some object which might be much larger when the array
is embedded in a larger object. 

The 'static' keyword only garantuees that the pointer points
to an array of a certain minimum size. So this can be used
to check that arrays of the required size are passed to
a function (and clang already emits a warning at a compile
time when it detects this). But 'static' does not mean
that accesses after the specified size are out-of-bounds
(at they would be for proper arrays). To achieve this
a language extension would be required. 

Martin