public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
@ 2017-04-29 22:10 Martin Sebor
       [not found] ` <656ca1db-1082-b1ed-a911-ba7bf48f09c0@redhat.com>
       [not found] ` <alpine.DEB.2.20.1704302338540.1461@digraph.polyomino.org.uk>
  0 siblings, 2 replies; 46+ messages in thread
From: Martin Sebor @ 2017-04-29 22:10 UTC (permalink / raw)
  To: Gcc Patch List, Jason Merrill

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

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

[-- Attachment #2: gcc-80560.diff --]
[-- Type: text/x-patch, Size: 10905 bytes --]

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

^ permalink raw reply	[flat|nested] 46+ messages in thread
end of thread, other threads:[~2017-07-05 22:33 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-29 22:10 [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560) Martin Sebor
     [not found] ` <656ca1db-1082-b1ed-a911-ba7bf48f09c0@redhat.com>
2017-05-01 15:49   ` Jason Merrill
2017-05-11 20:03     ` Martin Sebor
2017-05-12  2:43       ` Martin Sebor
2017-05-17 11:53         ` Pedro Alves
2017-06-29 16:15       ` Jan Hubicka
2017-06-29 20:23         ` Martin Sebor
2017-06-29 22:34           ` Jan Hubicka
2017-06-30  0:16             ` Martin Sebor
2017-06-30  8:34           ` Richard Biener
2017-06-30 14:29             ` Martin Sebor
2017-07-04  9:33         ` Richard Earnshaw (lists)
2017-05-11 16:34   ` Martin Sebor
2017-05-11 16:57     ` Jakub Jelinek
2017-05-11 17:17       ` Martin Sebor
2017-05-16 19:46     ` Jason Merrill
2017-05-16 22:28       ` Martin Sebor
2017-05-19 19:14         ` Jason Merrill
2017-05-19 21:11           ` Martin Sebor
2017-05-19 21:56             ` Jason Merrill
2017-05-22  2:07               ` Martin Sebor
2017-05-22  6:07                 ` Jason Merrill
2017-05-24 20:28                   ` Martin Sebor
2017-05-24 20:48                     ` Martin Sebor
2017-05-24 21:36                       ` Jason Merrill
2017-05-28  5:02                         ` Martin Sebor
     [not found]                           ` <cc62e93c-3b49-8e2f-70b9-acdd013fe760@redhat.com>
2017-06-02 21:28                             ` Martin Sebor
2017-06-05  2:02                               ` Jason Merrill
2017-06-05  7:53                                 ` Jason Merrill
2017-06-05 16:07                                   ` Martin Sebor
2017-06-05 19:13                                     ` Martin Sebor
2017-06-06  1:53                                       ` Martin Sebor
2017-06-06 22:24                                         ` Martin Sebor
2017-06-08  1:09                                           ` Jason Merrill
2017-06-08 20:25                                             ` Martin Sebor
2017-06-12 21:36                                               ` Jason Merrill
2017-06-15 16:26                                                 ` Martin Sebor
2017-06-15 21:31                                                   ` Jason Merrill
2017-06-16  7:38                                                     ` Richard Biener
2017-06-16  7:40                                                       ` Richard Biener
2017-05-17  1:01       ` Pedro Alves
2017-05-17  1:57         ` Martin Sebor
2017-05-17 11:23           ` Pedro Alves
2017-07-05 20:58   ` Andrew Pinski
2017-07-05 22:33     ` Martin Sebor
     [not found] ` <alpine.DEB.2.20.1704302338540.1461@digraph.polyomino.org.uk>
2017-05-03 16:18   ` Martin Sebor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).