[PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor <msebor at gmail dot com>]

Calling memset, memcpy, or similar to write to an object of
a non-trivial type (such as one that defines a ctor or dtor,
or has such a member) can break the invariants otherwise
maintained by the class and cause undefined behavior.

The motivating example that prompted this work was a review of
a change that added to a plain old struct a new member with a ctor
and dtor (in this instance the member was of type std::vector).

To help catch problems of this sort some projects (such as GDB)
have apparently even devised their own clever solutions to detect
them: https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html.

The attached patch adds a new warning, -Wnon-trivial-memaccess,
that has GCC detect these mistakes.  The patch also fixes up
a handful of instances of the problem in GCC.  These instances
correspond to the two patterns below:

  struct A
  {
    void *p;
    void foo (int n) { p = malloc (n); }
    ~A () { free (p); }
  };

  void init (A *a)
  {
    memset (a, 0, sizeof *a);
  }

and

  struct B
  {
    int i;
    ~A ();
  };

  void copy (B *p, const B *q)
  {
    memcpy (p, q, sizeof *p);
    ...
   }

These aren't undefined and the patch could be tweaked to allow
them.  I decided not to invest effort into it because, although
not strictly erroneous, I think they represent poor practice.
The code would be more clearly (and more in the spirit of "good"
C++) written in terms of the default constructor and assignment
operator.  The first one like so:

  *a = A ();

and the second one like so:

  *b = *q;

Martin

PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wnon-trivial-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (maybe_warn_nontrivial_memaccess): New function.
	(build_cxx_call): Call it.

gcc/ChangeLog:

	PR c++/80560
	* doc/invoke.texi (Wnon-trivial-memaccess): Document new option.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wnon-trivial-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9ad2f6e..dae5e80 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -792,6 +792,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wnon-trivial-memaccess
+C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for raw memory writes to objects of non-trivial types.
+
 Wnon-virtual-dtor
 C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
 Warn about non-virtual destructors.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c15b8e4..8655b53 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8152,6 +8152,43 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   return call;
 }
 
+/* Issue a warning on a call to the built-in function FNDECL if it is
+   a memory write whose destination is an object of a non-trivial type.  */
+
+static void
+maybe_warn_nontrivial_memaccess (location_t loc, tree fndecl, tree dest)
+{
+  /* Warn about raw memory operations whose destination is an object
+     of a non-trivial type because they are undefined.  */
+  bool memfunc = false;
+  switch (DECL_FUNCTION_CODE (fndecl))
+    {
+    case BUILT_IN_BZERO:
+    case BUILT_IN_MEMCPY:
+    case BUILT_IN_MEMMOVE:
+    case BUILT_IN_MEMPCPY:
+    case BUILT_IN_MEMSET:
+      memfunc = true;
+      break;
+
+    default:
+      break;
+    }
+
+  if (memfunc)
+    {
+      if (TREE_CODE (dest) == NOP_EXPR)
+	dest = TREE_OPERAND (dest, 0);
+
+      tree desttype = TREE_TYPE (TREE_TYPE (dest));
+
+      if (COMPLETE_TYPE_P (desttype) && !trivial_type_p (desttype))
+	warning_at (loc, OPT_Wnon_trivial_memaccess,
+		    "calling %qD with a pointer to a non-trivial type %#qT",
+		    fndecl, desttype);
+    }
+}
+
 /* Build and return a call to FN, using NARGS arguments in ARGARRAY.
    This function performs no overload resolution, conversion, or other
    high-level operations.  */
@@ -8184,6 +8221,9 @@ build_cxx_call (tree fn, int nargs, tree *argarray,
       if (!check_builtin_function_arguments (EXPR_LOCATION (fn), vNULL, fndecl,
 					     nargs, argarray))
 	return error_mark_node;
+
+      /* Warn if the built-in writes to an object of a non-trivial type.  */
+      maybe_warn_nontrivial_memaccess (loc, fndecl, argarray[0]);
     }
 
     /* If it is a built-in array notation function, then the return type of
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 0eeea7b..e1d01a9 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -215,7 +215,8 @@ in the following sections.
 -Wabi=@var{n}  -Wabi-tag  -Wconversion-null  -Wctor-dtor-privacy @gol
 -Wdelete-non-virtual-dtor  -Wliteral-suffix  -Wmultiple-inheritance @gol
 -Wnamespaces  -Wnarrowing @gol
--Wnoexcept  -Wnoexcept-type  -Wnon-virtual-dtor  -Wreorder  -Wregister @gol
+-Wnoexcept  -Wnoexcept-type  -Wnon-trivial-memaccess @gol
+-Wnon-virtual-dtor  -Wreorder  -Wregister @gol
 -Weffc++  -Wstrict-null-sentinel  -Wtemplates @gol
 -Wno-non-template-friend  -Wold-style-cast @gol
 -Woverloaded-virtual  -Wno-pmf-conversions @gol
@@ -2911,6 +2912,21 @@ void g() noexcept;
 void h() @{ f(g); @} // in C++14 calls f<void(*)()>, in C++1z calls f<void(*)()noexcept>
 @end smallexample
 
+@item -Wnon-trivial-memaccess @r{(C++ and Objective-C++ only)}
+@opindex Wnon-trivial-memaccess
+Warn when the destination of a call to a raw memory function such as
+@code{memset} or @code{memcpy} is an object of a non-trivial class type.
+Modifying the representation of such an object may violate invariants
+maintained by member functions of the class.
+For example, the call to @code{memset} below is undefined becase it
+modifies a non-trivial class object and is, therefore, diagnosed.
+The safe way to either initialize or "reset" objects of non-trivial
+types is by using the appropriate constructor.
+@smallexample
+std::string str = "abc";
+memset (&str, 0, 3);
+@end smallexample
+The @option{-Wnon-trivial-memaccess} option is enabled by @option{-Wall}.
 
 @item -Wnon-virtual-dtor @r{(C++ and Objective-C++ only)}
 @opindex Wnon-virtual-dtor
diff --git a/gcc/testsuite/g++.dg/Wnon-trivial-memaccess.C b/gcc/testsuite/g++.dg/Wnon-trivial-memaccess.C
new file mode 100644
index 0000000..812a9eb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wnon-trivial-memaccess.C
@@ -0,0 +1,122 @@
+/* PR c++/80560 - warn on undefined memory operations involving non-trivial
+   types
+   { dg-do compile }
+   { dg-options "-Wnon-trivial-memaccess" } */
+
+struct Trivial { int i; char *s; char a[4]; };
+struct HasDefaultCtor { HasDefaultCtor (); };
+struct HasCopyCtor { HasCopyCtor (); };
+struct HasDtor { HasDtor (); };
+struct HasAssign { void operator= (HasAssign&); };
+
+typedef __SIZE_TYPE__ size_t;
+
+extern "C" {
+  void bzero (void*, size_t);
+  void* memcpy (void*, const void*, size_t);
+  void* memmove (void*, const void*, size_t);
+  void* mempcpy (void*, const void*, size_t);
+  void* memset (void*, int, size_t);
+}
+
+void sink (void*);
+
+#define T(fn, arglist) (fn arglist, sink (p))
+
+void test (Trivial *p, void *q)
+{
+  T (bzero, (p, sizeof *p));
+  T (bzero, (q, sizeof *p));
+
+  T (memcpy, (p, q, sizeof *p));
+  T (memcpy, (q, p, sizeof *p));
+
+  T (memset, (p, 0, sizeof *p));
+  T (memset, (q, 0, sizeof *p));
+
+  T (memmove, (p, q, sizeof *p));
+  T (memmove, (q, p, sizeof *p));
+}
+
+void test (HasDefaultCtor *p, const void *q)
+{
+  T (bzero, (p, sizeof *p));   // { dg-warning "calling .void bzero(\[^\n\r\]*). with a pointer to a non-trivial type 'struct HasDefaultCtor." }
+
+  T (memcpy, (p, q, sizeof *p));  // { dg-warning "calling .void\\* memcpy" }
+
+  T (memset, (p, 0, sizeof *p));  // { dg-warning "calling .void\\* memset" }
+
+  T (memmove, (p, q, sizeof *p)); // { dg-warning "calling .void\\* memmove" }
+
+  T (mempcpy, (p, q, sizeof *p)); // { dg-warning "calling .void\\* mempcpy" }
+}
+
+void test (HasCopyCtor *p, const void *q)
+{
+  T (bzero, (p, sizeof *p));      // { dg-warning "calling .void bzero" }
+
+  T (memcpy, (p, q, sizeof *p));  // { dg-warning "calling .void\\* memcpy" }
+
+  T (memset, (p, 0, sizeof *p));  // { dg-warning "calling .void\\* memset" }
+
+  T (memmove, (p, q, sizeof *p)); // { dg-warning "calling .void\\* memmove" }
+
+  T (mempcpy, (p, q, sizeof *p)); // { dg-warning "calling .void\\* mempcpy" }
+}
+
+void test (HasDtor *p, const void *q)
+{
+  T (bzero, (p, sizeof *p));      // { dg-warning "calling .void bzero" }
+
+  T (memcpy, (p, q, sizeof *p));  // { dg-warning "calling .void\\* memcpy" }
+
+  T (memset, (p, 0, sizeof *p));  // { dg-warning "calling .void\\* memset" }
+
+  T (memmove, (p, q, sizeof *p)); // { dg-warning "calling .void\\* memmove" }
+
+  T (mempcpy, (p, q, sizeof *p)); // { dg-warning "calling .void\\* mempcpy" }
+}
+
+void test (HasAssign *p, const void *q)
+{
+  T (bzero, (p, sizeof *p));     // { dg-warning "calling .void bzero" }
+
+  T (memcpy, (p, q, sizeof *p));  // { dg-warning "calling .void\\* memcpy" }
+
+  T (memset, (p, 0, sizeof *p));  // { dg-warning "calling .void\\* memset" }
+
+  T (memmove, (p, q, sizeof *p)); // { dg-warning "calling .void\\* memmove" }
+
+  T (mempcpy, (p, q, sizeof *p)); // { dg-warning "calling .void\\* mempcpy" }
+}
+
+
+void test_expr (int i)
+{
+  struct TestClass: HasDefaultCtor { };
+  TestClass a, b;
+
+  static void *p;
+
+  T (bzero, (i < 0 ? &a : &b, 1));  // { dg-warning "calling .void bzero" }
+}
+
+void test_this ()
+{
+#undef T
+#define T(fn, arglist) (fn arglist, sink (this))
+
+  static const void *p;
+
+  struct TestDefaultCtor: HasDefaultCtor
+  {
+    TestDefaultCtor ()
+    {
+      T (bzero, (this, sizeof *this)); // { dg-warning "calling .void bzero" }
+      T (memset, (this, 0, sizeof *this)); // { dg-warning "calling .void\\* memset" }
+      T (memcpy, (this, p, sizeof *this)); // { dg-warning "calling .void\\* memcpy" }
+      T (memmove, (this, p, sizeof *this)); // { dg-warning "calling .void\\* memmove" }
+      T (mempcpy, (this, p, sizeof *this)); // { dg-warning "calling .void\\* mempcpy" }
+    }
+  };
+}
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 949489e..4e36e38 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -62,7 +62,8 @@ extern unsigned num_macro_tokens_counter;
 
 line_maps::~line_maps ()
 {
-  htab_delete (location_adhoc_data_map.htab);
+  if (location_adhoc_data_map.htab)
+    htab_delete (location_adhoc_data_map.htab);
 }
 
 /* Hash function for location_adhoc_data hashtable.  */
@@ -347,7 +348,7 @@ void
 linemap_init (struct line_maps *set,
 	      source_location builtin_location)
 {
-  memset (set, 0, sizeof (struct line_maps));
+  *set = line_maps ();
   set->highest_location = RESERVED_LOCATION_COUNT - 1;
   set->highest_line = RESERVED_LOCATION_COUNT - 1;
   set->location_adhoc_data_map.htab =
diff --git a/libitm/beginend.cc b/libitm/beginend.cc
index d04f3e9..c6550a3 100644
--- a/libitm/beginend.cc
+++ b/libitm/beginend.cc
@@ -431,7 +431,7 @@ GTM::gtm_transaction_cp::save(gtm_thread* tx)
   // Save everything that we might have to restore on restarts or aborts.
   jb = tx->jb;
   undolog_size = tx->undolog.size();
-  memcpy(&alloc_actions, &tx->alloc_actions, sizeof(alloc_actions));
+  alloc_actions = tx->alloc_actions;
   user_actions_size = tx->user_actions.size();
   id = tx->id;
   prop = tx->prop;
@@ -449,7 +449,7 @@ GTM::gtm_transaction_cp::commit(gtm_thread* tx)
   // commits of nested transactions. Allocation actions must be committed
   // before committing the snapshot.
   tx->jb = jb;
-  memcpy(&tx->alloc_actions, &alloc_actions, sizeof(alloc_actions));
+  tx->alloc_actions = alloc_actions;
   tx->id = id;
   tx->prop = prop;
 }
@@ -485,7 +485,7 @@ GTM::gtm_thread::rollback (gtm_transaction_cp *cp, bool aborting)
       prop = cp->prop;
       if (cp->disp != abi_disp())
 	set_abi_disp(cp->disp);
-      memcpy(&alloc_actions, &cp->alloc_actions, sizeof(alloc_actions));
+      alloc_actions = cp->alloc_actions;
       nesting = cp->nesting;
     }
   else