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

  This patch catches invalid initialization of non-POD types with
memset, at compile time.

This is what I used to catch the problems fixed by the rest of the
series:

  $ make -k 2>&1 | grep "deleted function"
  src/gdb/breakpoint.c:951:53: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
  src/gdb/breakpoint.c:7325:32: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
  src/gdb/btrace.c:1153:42: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = btrace_insn; <template-parameter-1-2> = void; size_t = long unsigned int]’

I'll move this to the end of the series before pushing (if agreed).

(I've posted another series recently that adds some of the same traits
bits to common/traits.h.  They're really useful.)

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* common/common-defs.h: Include "common/poison.h".
	* common/function-view.h: (Not, Or, Requires): Move to traits.h.
	* common/poison.h: New file.
	* common/traits.h: Include <type_traits>.
	(Not, Or, Requires): New, moved from common/function-view.h.
	(And): New.
---
 gdb/common/common-defs.h   |  1 +
 gdb/common/function-view.h | 40 +++-------------------
 gdb/common/poison.h        | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/traits.h        | 55 ++++++++++++++++++++++++++++++
 4 files changed, 143 insertions(+), 36 deletions(-)
 create mode 100644 gdb/common/poison.h
  

On 2017-04-12 22:27, Pedro Alves wrote:
> This patch catches invalid initialization of non-POD types with
> memset, at compile time.
> 
> This is what I used to catch the problems fixed by the rest of the
> series:
> 
>   $ make -k 2>&1 | grep "deleted function"
>   src/gdb/breakpoint.c:951:53: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = bp_location;
> <template-parameter-1-2> = void; size_t = long unsigned int]’
>   src/gdb/breakpoint.c:7325:32: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = bp_location;
> <template-parameter-1-2> = void; size_t = long unsigned int]’
>   src/gdb/btrace.c:1153:42: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = btrace_insn;
> <template-parameter-1-2> = void; size_t = long unsigned int]’
> 
> I'll move this to the end of the series before pushing (if agreed).
> 
> (I've posted another series recently that adds some of the same traits
> bits to common/traits.h.  They're really useful.)

That's really nice.  I'm actually surprised we didn't get random crashes 
because of that yet!

> diff --git a/gdb/common/poison.h b/gdb/common/poison.h
> new file mode 100644
> index 0000000..57a1733
> --- /dev/null
> +++ b/gdb/common/poison.h
> @@ -0,0 +1,83 @@
> +/* Poison symbols at compile time.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef COMMON_POISON_H
> +#define COMMON_POISON_H
> +
> +#include "traits.h"
> +
> +/* Poison memset of non-POD types.  The idea is catching invalid
> +   initialization of non-POD structs that is easy to be introduced as
> +   side effect of refactoring.  For example, say this:
> +
> + struct S { VEC(foo_s) *m_data; };
> +
> +is converted to this at some point:
> +
> + struct S {
> +   S() { m_data.reserve (10); }
> +   std::vector<foo> m_data;
> + };

Here it says struct S ...

> +
> +and old code was initializing B objects like this:
> +
> + struct B b;
> + memset (&b, 0, sizeof (B)); // whoops, now wipes vector.

... and here struct B?

Simon
  

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.
  

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.
  

Hi Simon,

Sorry for the delay.  Finally managed to get back to this.

On 04/24/2017 02:12 AM, 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.

I think that that's a great idea!  I tried that locally and see that
this already catches two bad cases (btrace_function and objfile).

We don't need to use non-standard statement expressions though.
Function templates should work just as well here:

template<typename T>
T *xnew ()
{
  static_assert (std::is_pod<T>::value, "use operator new instead");
  return (T *) xmalloc (sizeof (T));
}

template<typename T>
T *xcnew ()
{
  static_assert (std::is_pod<T>::value, "use operator new instead");
  return (T *) xcalloc (1, sizeof (T));
}

#undef XNEW
#define XNEW(T) xnew<T>()
#undef XCNEW
#define XCNEW(T) xcnew<T>()

As should lambdas:

#undef XNEW
#define XNEW(T) [] () -> T *						\
  {									\
    static_assert (std::is_pod<T>::value, "use operator new instead");	\
    return (T *) xmalloc (sizeof (T));					\
  } ()

#undef XCNEW
#define XCNEW(T) [] () -> T *						\
  {									\
    static_assert (std::is_pod<T>::value, "use operator new instead");	\
    return (T *) xcalloc (1, sizeof (T));				\
  } ()

I think the template version is likely a little bit easier
to understand and debug (e.g., easy to put a breakpoint on the function
template, not so easy to put a breakpoint on a lambda).  I'd just
confirm that the template/lambda is completely optimized out on an
optimized build (e.g., compare out of "$ size gdb" before and after
patch).

Thanks,
Pedro Alves
  

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
  

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
  

On 04/30/2017 02:51 AM, Simon Marchi wrote:

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

Yeah.  Still, better than nothing.

BTW, GCC ran into similar issues almost at the same time
we started discussing this, and I've been discussing
with the GCC folks about a new GCC warning that flags invalid
memcpy/memset misuses.  Martin Sebor has been working on a patch
and it's getting close to be merged, AFAICT.

See:
 https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01527.html
First version of the GCC patch here:
 https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01571.html
Discussion crossed month boundary here:
 https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00925.html
Latest patch is here:
 https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00976.html

I won't be a full replacement since we'll still want our
poisoning for other functions (xmalloc, xfree, etc.).  And
then there's current/older gccs.  But still, pretty neat, IMO.

Thanks,
Pedro Alves
  

On 2017-05-17 07:35, Pedro Alves wrote:
> On 04/30/2017 02:51 AM, Simon Marchi wrote:
> 
>> 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".
> 
> Yeah.  Still, better than nothing.
> 
> BTW, GCC ran into similar issues almost at the same time
> we started discussing this, and I've been discussing
> with the GCC folks about a new GCC warning that flags invalid
> memcpy/memset misuses.  Martin Sebor has been working on a patch
> and it's getting close to be merged, AFAICT.
> 
> See:
>  https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01527.html
> First version of the GCC patch here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01571.html
> Discussion crossed month boundary here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00925.html
> Latest patch is here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00976.html
> 
> I won't be a full replacement since we'll still want our
> poisoning for other functions (xmalloc, xfree, etc.).  And
> then there's current/older gccs.  But still, pretty neat, IMO.

Thanks for the info!

I have a branch in progress about poisoning XNEW and friends:
https://github.com/simark/binutils-gdb/commits/poison-xnew

I won't have time to look at it until at least next week, if anybody 
wants to pick it up, they are free to do so.

Simon
  

On 05/17/2017 02:11 PM, Simon Marchi wrote:

> I have a branch in progress about poisoning XNEW and friends:
> https://github.com/simark/binutils-gdb/commits/poison-xnew

Nice!

> 
> I won't have time to look at it until at least next week, if anybody
> wants to pick it up, they are free to do so.

I won't have time either, so I'll just wait.  :-)

Thanks,
Pedro Alves