Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-04-27 09:58, Pedro Alves wrote:
> On 04/24/2017 02:53 AM, Simon Marchi wrote:
>> Actually, it should probably use std::is_trivially_constructible.
>> And I
>> suppose we could do the same with xfree, delete it when
>> !std::is_trivially_destructible.
> 
> 
> I think you wanted std::is_trivially_default_constructible
> for XNEW.

 From what I understand, using is_trivially_default_constructible<T> is 
the same as is_trivially_constructible<T>.  We can of course use 
is_trivially_default_constructible if it's clearer.

> I think that we want _both_ conditions (*constructible
> and *destructible) on both XNEW and xfree.  For example, it'll be
> good to catch the mismatching new/delete that could sneak in otherwise:

That seems reasonnable.  We want to "upgrade" to new and delete in a 
lock step anyway.

>  // type with trivial constructor
>  struct A
>  {
>    // A() = default;
>    ~A() { /* do something with side effects */ } // not trivial
>  };
> 
>  // type with trivial destructor
>  struct B
>  {
>    B() { /* do something with side effects */ } // not trivial
>    //~B() = default;
>  };
> 
>  void foo ()
>  {
>    A *a = XNEW (struct A);
>    delete a;
>    B *b = new B;
>    xfree (b);
>  }
> 
> Calling delete on a pointer not allocated with new is undefined 
> behavior.
> These mismatches are also flagged by -fsanitize=address, but
> making them compile-time errors would be even better.
> 
> This wouldn't catch allocating types that are both trivially
> default constructible and trivially destructible, and which _also_
> have non-default ctors, like this, for example:
> 
>  struct C
>  {
>    C() = default;
>    explicit C(int) { /* some side effects */ }
>  };
> 
>  static_assert (std::is_trivially_default_constructible<C>::value, "");
>  static_assert (std::is_trivially_destructible<C>::value, "");
> 
>  C *b = new C(1);
>  xfree (b); // whoops, technically undefined.  -fsanitify=address
> likely complains.
> 
> but std::is_pod wouldn't either.
> 
> If we make a type non-standard-layout, then it no longer is POD:
> 
>  struct D
>  {
>   // Mix of public/private fields => not POD
>  public:
>    int a;
>  private:
>    int b;
>  };
> 
> This (D) case is likely to not really be problematic in practice WRT
> to allocation/deallocation with malloc/free, but it still feels
> like a code smell to me.  I'd be willing to try forcing use
> of new/delete for these types too.  This would suggest using the
> bigger std::is_pod hammer in XNEW/xfree instead of just
> std::is_trivially_*ctible.  But I'd understand if others disagree.

I think it would be a good guideline to use new/delete for types that 
have some C++-related stuff in them, even if it's not technically 
necessary.

Note that this won't be bulletproof also because at many places xfree is 
used on a void pointer, so we don't know what we're really free'ing.  In 
some other cases, objects are freed using a pointer to their "C base 
class".

Simon