[PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)

[Martin Sebor]

On 01/18/2017 01:10 AM, Jakub Jelinek wrote:
> On Tue, Jan 17, 2017 at 10:59:43PM -0700, Jeff Law wrote:
>>> I agree that breaking those applications would be bad.  It could
>>> be dealt with by adding an option to let them disable the insertion
>>> of the trap.  With the warning, programmers would get a heads up
>>> that their (already dubious) code won't work otherwise.  I don't
>>> think it's a necessary or wise to have the default mode be the most
>>> permissive (and most dangerous) and expect users to tweak options
>>> to make it safe.  Rather, I would argue that it should be the other
>>> way around.  Make the default safe and strict and let the advanced
>>> users who know how deal with the risks tweak those options.
>> I still come back to the assertion that changing this loop to a mem* is
>> fundamentally the wrong thing to do as it changes something that has well
>> defined semantics to something that is invalid.
>>
>> Thus the transformation into a mem* call is invalid.
>
> The mem* call is as valid as the loop, it will work exactly the same.

The problem here isn't the loop itself or the memset call but the
bounds computed from the inputs.

In the test case submitted with the bug the inputs supplied by
the program are well within a valid range, yet the bounds of
the loop that are computed by GCC from those inputs are excessive
because they are based on impossible ranges (or an incomplete view
of the program).

There is no unbounded loop in

   void f(std::vector<int> &v)
   {
     size_t n = v.size ();

     if (n > 1 && n < 5)
       v.resize (n - 1);
   }

yet GCC synthesizes one:

   void f(std::vector<int>&) (struct vector & v)
   {
     ...
     <bb 2> [100.00%]:
     _7 = MEM[(int * *)v_5(D)];             // v._M_impl._M_start
     _8 = MEM[(int * *)v_5(D) + 8B];        // v._M_impl._M_finish
     _9 = (long int) _8;
     _10 = (long int) _7;
     _11 = _9 - _10;
     _12 = _11 /[ex] 4;
     _13 = (long unsigned int) _12;         // v.size()
     _1 = _13 + 18446744073709551614;       // v.size() - 4
     if (_1 <= 2)                           // if (v.size() <= 6)
       goto <bb 3>; [36.64%]
     else
       goto <bb 9>; [63.36%]

     <bb 3> [36.64%]:
     _2 = _13 + 18446744073709551615;       // v.size() - 1
     if (_2 > _13)                          // if (v.size() == 0)
       goto <bb 4>; [29.56%]                //   this can't happen
     else
       goto <bb 7>; [70.44%]

     <bb 4> [5.85%]:
     _24 = v_5(D)->D.15828._M_impl._M_end_of_storage;
     _25 = (long int) _24;
     _28 = _25 - _9;                        // bytes left
     if (_28 == -4)                         // this can't happen
       goto <bb 5>; [67.00%]
     else
       goto <bb 6>; [33.00%]

     <bb 5> [3.92%]:
     __builtin_memset (_8, 0, 18446744073709551612);   // (size_t)-4
     ...

     <bb 6> [1.93%]:
     std::__throw_length_error ("vector::_M_default_append");

> If you have say on 32-bit target
> #include <stdlib.h>
>
> int
> main ()
> {
>   char *p = malloc (3U * 1024 * 1024 * 1024);
>   if (p == NULL)
>     return 0;
>   size_t i;
>   for (i = 0; i < 3U * 1024 * 1024 * 1024; i++)
>     p[i] = 6;
>   use (p);
>   return 0;
> }

Unlike in the test case submitted with the bug, here the loop is in
the source and warning on it would be justified for most programs.

Here's a test case that's closer to the one from the bug.  It also
ends up with the out of bounds memset even at -O1, during PRE.

typedef __SIZE_TYPE__ size_t;

struct S
   int *p0, *p1, *p2;

   size_t size () const { return p1 - p0; }

   void f (size_t n) {
     if (n > size ())       // can't happen because
       foo (n - size ());   //   n is in [1, MIN(size() - 1, 3)]
     else if (n < size ())
       bar (p0 + n);
   }

   void foo (size_t n)
   {
     size_t left = (size_t)(p2 - p1);
     if (left >= n)
       __builtin_memset (p2, 0, n * sizeof *p2);
   }

   void bar (int*);
};

void f (S &s)
{
   size_t n = s.size ();
   if (n > 1 && n < 5)
     s.f (n - 1);
}

Martin