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

[Pedro Alves <palves@redhat.com>]

On 04/24/2017 02:53 AM, Simon Marchi wrote:
> On 2017-04-23 21:12, Simon Marchi wrote:
>> On 2017-04-12 22:27, Pedro Alves wrote:
>>> This patch catches invalid initialization of non-POD types with
>>> memset, at compile time.
>>
>> Would it be possible to do something similar but to catch uses of
>> XNEW/XCNEW with types that need new?  XNEW is defined as:
>>
>> #define XNEW(T) ((T *) xmalloc (sizeof (T)))
>>
>> I just tried this, and it seems to work well:
>>
>> #define assert_pod(T) static_assert(std::is_pod<T>::value)
>>
>> #undef XNEW
>> #define XNEW(T) ({ assert_pod(T); (T *) xmalloc (sizeof (T)); })
>> #undef XCNEW
>> #define XCNEW(T)  ({ assert_pod(T); (T *) xcalloc (1, sizeof (T)); })
>>
>> assuming the compiler knows about statement expressions.
> 
> 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.  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:

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

Thanks,
Pedro Alves