[1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
Commit Message
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
Comments
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
@@ -82,6 +82,7 @@
#include "common-debug.h"
#include "cleanups.h"
#include "common-exceptions.h"
+#include "common/poison.h"
#define EXTERN_C extern "C"
#define EXTERN_C_PUSH extern "C" {
@@ -153,34 +153,6 @@
namespace gdb {
-namespace traits {
- /* A few trait helpers. */
- template<typename Predicate>
- struct Not : public std::integral_constant<bool, !Predicate::value>
- {};
-
- template<typename...>
- struct Or;
-
- template<>
- struct Or<> : public std::false_type
- {};
-
- template<typename B1>
- struct Or<B1> : public B1
- {};
-
- template<typename B1, typename B2>
- struct Or<B1, B2>
- : public std::conditional<B1::value, B1, B2>::type
- {};
-
- template<typename B1,typename B2,typename B3, typename... Bn>
- struct Or<B1, B2, B3, Bn...>
- : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type
- {};
-} /* namespace traits */
-
namespace fv_detail {
/* Bits shared by all function_view instantiations that do not depend
on the template parameters. */
@@ -209,9 +181,9 @@ class function_view<Res (Args...)>
{
template<typename From, typename To>
using CompatibleReturnType
- = traits::Or<std::is_void<To>,
- std::is_same<From, To>,
- std::is_convertible<From, To>>;
+ = gdb::Or<std::is_void<To>,
+ std::is_same<From, To>,
+ std::is_convertible<From, To>>;
/* True if Func can be called with Args, and either the result is
Res, convertible to Res or Res is void. */
@@ -227,10 +199,6 @@ class function_view<Res (Args...)>
: std::is_same<function_view, typename std::decay<Callable>::type>
{};
- /* Helper to make SFINAE logic easier to read. */
- template<typename Condition>
- using Requires = typename std::enable_if<Condition::value, void>::type;
-
public:
/* NULL by default. */
@@ -248,7 +216,7 @@ class function_view<Res (Args...)>
compatible. */
template
<typename Callable,
- typename = Requires<traits::Not<IsFunctionView<Callable>>>,
+ typename = Requires<gdb::Not<IsFunctionView<Callable>>>,
typename = Requires<IsCompatibleCallable<Callable>>>
function_view (Callable &&callable) noexcept
{
new file mode 100644
@@ -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;
+ };
+
+and old code was initializing B objects like this:
+
+ struct B b;
+ memset (&b, 0, sizeof (B)); // whoops, now wipes vector.
+
+Declaring memset as deleted for non-POD types makes the memset above
+be a compile-time error. */
+
+/* Helper for SFINAE. True if "T *" is memsettable. I.e., if T is
+ either void, or POD. */
+template<typename T>
+struct IsMemsettable
+ : gdb::Or<std::is_void<T>,
+ std::is_pod<T>>
+{};
+
+template <typename T,
+ typename = gdb::Requires<gdb::Not<IsMemsettable<T>>>>
+void *memset (T *s, int c, size_t n) = delete;
+
+/* Similarly, poison memcpy and memmove of non trivially-copyable
+ types, which is undefined. */
+
+/* True if "T *" is relocatable. I.e., copyable with memcpy/memmove.
+ I.e., T is either trivially copyable, or void. */
+template<typename T>
+struct IsRelocatable
+ : gdb::Or<std::is_void<T>,
+ std::is_trivially_copyable<T>>
+{};
+
+/* True if both source and destination are relocatable. */
+
+template <typename D, typename S>
+using BothAreRelocatable
+ = gdb::And<IsRelocatable<D>, IsRelocatable<S>>;
+
+template <typename D, typename S,
+ typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
+void *memcpy (D *dest, const S *src, size_t n) = delete;
+
+template <typename D, typename S,
+ typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
+void *memmove (D *dest, const S *src, size_t n) = delete;
+
+#endif /* COMMON_POISON_H */
@@ -18,6 +18,8 @@
#ifndef COMMON_TRAITS_H
#define COMMON_TRAITS_H
+#include <type_traits>
+
namespace gdb {
/* Pre C++14-safe (CWG 1558) version of C++17's std::void_t. See
@@ -29,6 +31,59 @@ struct make_void { typedef void type; };
template<typename... Ts>
using void_t = typename make_void<Ts...>::type;
+/* A few trait helpers, mainly stolen from libstdc++. Uppercase
+ because "and/or", etc. are reserved keywords. */
+
+template<typename Predicate>
+struct Not : public std::integral_constant<bool, !Predicate::value>
+{};
+
+template<typename...>
+struct Or;
+
+template<>
+struct Or<> : public std::false_type
+{};
+
+template<typename B1>
+struct Or<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct Or<B1, B2>
+ : public std::conditional<B1::value, B1, B2>::type
+{};
+
+template<typename B1,typename B2,typename B3, typename... Bn>
+struct Or<B1, B2, B3, Bn...>
+ : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type
+{};
+
+template<typename...>
+struct And;
+
+template<>
+struct And<> : public std::true_type
+{};
+
+template<typename B1>
+struct And<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct And<B1, B2>
+ : public std::conditional<B1::value, B2, B1>::type
+{};
+
+template<typename B1, typename B2, typename B3, typename... Bn>
+struct And<B1, B2, B3, Bn...>
+ : public std::conditional<B1::value, And<B2, B3, Bn...>, B1>::type
+{};
+
+/* Helper to make SFINAE logic easier to read. */
+template<typename Condition>
+using Requires = typename std::enable_if<Condition::value, void>::type;
+
}
#endif /* COMMON_TRAITS_H */