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

> 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.

>
> I think all modes.

Sounds good.  I've enabled it in all modes.

> The if (1) seems left over from development.

Right.

>
> It looks like this will multiply *cst_size by the sub-array length once
> for each element of the outer array, leading to a too-large result.  And
> also generate redundant code to check the bounds of the sub-array
> multiple times.

Great catch, thank you!  I think it was a mistake on my part to
try to build both kinds of checks in the same function.  In the
update patch I split it up into two: one to build the bounds
check and another to build the initializer check.

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.
If you think it's worthwhile, I can see about implementing the
runtime reshaping in stage 1.

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

> 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.

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.

>
>> +          /* 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.
As much as I'd like to fix this I'm concerned that it might
take me too long and don't want to hold up the schedule or
risk that the patch, incomplete though it may be, won't make
it into GCC 6.

Attached is the latest update.  I've taken the liberty of
setting the limit to 1 MB in hopes you'll find it acceptable.
I can easily bump it up to an even more permissive value,
though as I mention above, I'd prefer not to lift altogether.

I'm happy to continue improving the patch but since time is
running out I want to post an updated version for consideration
as is and get feedback on it.

Thanks
Martin

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

Commit Message

Comments

Patch