Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill <jason at redhat dot com>]

On 04/01/2016 07:02 PM, Martin Sebor wrote:
> > Fair enough.  I don't think we can impose an arbitrary 64K limit,
> > however, as that is a lot smaller than the 8MB default stack size, and
> > programs can use setrlimit to increase the stack farther.  For GCC 6 let
> > not impose any limit beyond non-negative/overflowing, and as you say we
> > can do something better in GCC 7.
>
> Would you be open to imposing a somewhat more permissive limit,
> say on the order of one or a few megabytes (but less than the
> default 8 MB on Linux)?
>
> I ask because I expect the majority of programmer errors with
> VLAs to be due to out-of-range bounds that couldn't be
> accommodated even if stack space was extended well beyond
> the Linux default (say hundreds of MB), or that would result
> in complete stack space exhaustion.  I.e., not caused by
> deliberately trying to create very large VLAs but rather by
> accidentally creating VLAs with unconstrained bounds (due to
> a failure to validate input, uninitialized unsigned variables,
> etc.)
>
> I expect fewer cases to be due to negative or zero bounds or
> excessive initializers.
>
> I also expect programmers to want to find out about such bugs
> sooner (i.e., in unit testing with plentiful stack space) rather
> than after software has been deployed (and under low stack space
> conditions not exercised during unit testing).
>
> To that end, I think a lower limit is going to be more helpful
> than a permissive one (or none at all).

> But if even a few MB seems too strict, I would find having even
> an exceedingly liberal limit (say 1GB) much preferable to none
> at all as it makes it possible to exercise boundary conditions
> such as the size overflow problem you noted below.

That sounds reasonable, as long as users with unusual needs can adjust 
it with a flag, but even so I'm nervous about doing this in stage 4.  It 
certainly isn't a regression.

> During additional testing it dawned on me that there is no good
> way to validate (or even to initialize) the initializer list of
> a multi-dimensional VLA that isn't unambiguously braced.
>
> For example, the following VLA with N = 2 and N = 3:
>
>      int A [M][N] = { 1, 2, 3, 4 };
>
> Unpatched, GCC initializes it to { { 1, 2, 3 }, { 0, 0, 0 } }.
> With my first patch, GCC throws.  Neither is correct, but doing
> the right thing would involve emitting parameterizing the
> initialization code for the value of each bound.  While that
> might be doable it feels like a bigger change than I would be
> comfortable attempting at this stage.  To avoid the problem I've
> made it an error to specify a partially braced VLA initializer.

Sounds good.

> If you think it's worthwhile, I can see about implementing the
> runtime reshaping in stage 1.

No, thanks.  I think requiring explicit bracing is fine.

> > It seems to me that we want use the existing check for excess
> > initializers in build_vec_init, in the if (length_check) section, though
> > as you mention in 70019 that needs to be improved to handle STRING_CST.
>
> I don't think modifying build_vec_init() alone would be sufficient.
> For example, the function isn't called for a VLA with a constant
> bound like this one:
>
>   int A [2][N] = { 1, 2, 3, 4 };

That seems like a bug, due to array_of_runtime_bound_p returning false 
for that array.

> > Also, I think we should check for invalid bounds in
> > compute_array_index_type, next to the UBsan code.  Checking bounds only
> > from cp_finish_decl means that we don't check uses of VLA types other
> > than variable declarations.
>
> You mean VLA typedefs?  That's true, though I have consciously
> avoided dealing with those.  They're outlawed in N3639 and so
> I've been focusing just on variables.  But since GCC accepts
> VLA typedefs too I was thinking I would bring them up at some
> point in the future to decide what to do about them.

And cast to pointer to VLAs.  But for non-variable cases we don't care 
about available stack, so we wouldn't want your allocation limit to apply.

> As for where to add the bounds checking code, I also at first
> thought of checking the bounds parallel to the UBSan code in
> compute_array_index_type() and even tried that approach. The
> problem with it is that it only considers one array dimension
> at a time, without the knowledge of the others.  As a result,
> as noted in sanitizer/70051, it doesn't correctly detect
> overflows in the bounds of multidimensional VLAs.

It doesn't, but I don't see why it couldn't.  It should be fine to check 
each dimension for overflow separately; if an inner dimension doesn't 
overflow, we can go on and consider the outer dimension.

Incidentally, I was wondering if it would make sense to use the 
overflowing calculation for both TYPE_SIZE and the sanity check when 
we're doing both.

> > > +          /* Avoid instrumenting constexpr functions.  Those must
> > > +         be checked statically, and the (non-constexpr) dynamic
> > > +         instrumentation would cause them to be rejected.  */

> > Hmm, this sounds wrong; constexpr functions can also be called with
> > non-constant arguments, and the instrumentation should be folded away
> > when evaluating a call with constant arguments.

> You're right that constexpr functions should be checked as
> well.  Unfortunately, at present, due to c++/70507 the check
> (or rather the call to __builtin_mul_overflow) isn't folded
> away and we end up with error: call to internal function.

Ah, sure.  It should be pretty simple to teach the constexpr code how to 
handle that built-in.

Jason