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

[Andrew Pinski]

On Sun, Apr 30, 2017 at 1:02 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Martin,
>
> Thanks much for doing this.  A few comments below, in light of my
> experience doing the equivalent checks in the gdb patch linked below,
> using standard C++11.
>
> On 04/29/2017 09:09 PM, Martin Sebor wrote:
>> 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 ();
>>   };
>
> (typo: "~B ();")
>
>>
>>   void copy (B *p, const B *q)
>>   {
>>     memcpy (p, q, sizeof *p);
>>     ...
>>    }
>>
>
> IMO the check should be relaxed from "type is trivial" to "type is
> trivially copyable" (which is what the gdb detection at
> https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
> uses for memcpy/memmove).  Checking that the destination is trivial is
> going to generate false positives -- specifically, [basic-types]/3
> specifies that it's fine to memcpy trivially _copyable_ types, not
> trivial types.  A type can be both non-trivial and trivially copyable
> at the same time.  For example, this compiles, but triggers
> your new warning:
>
> #include <stdlib.h>
> #include <string.h>
> #include <type_traits>
>
> struct NonTrivialButTriviallyCopyable
> {
>   NonTrivialButTriviallyCopyable () : i (0) {}
>   int i;
> };
>
> static_assert (!std::is_trivial<NonTrivialButTriviallyCopyable>::value, "");
> static_assert (std::is_trivially_copyable<NonTrivialButTriviallyCopyable>::value, "");
>
> void copy (NonTrivialButTriviallyCopyable *dst, NonTrivialButTriviallyCopyable *src)
> {
>   memcpy (dst, src, sizeof (*src));
> }
>
> $ /opt/gcc/bin/g++ -std=gnu++11 trivial-warn.cc -o trivial-warn -g3 -O0 -Wall -Wextra -c
> trivial-warn.cc: In function ‘void copy(NonTrivialButTriviallyCopyable*, NonTrivialButTriviallyCopyable*)’:
> trivial-warn.cc:16:34: warning: calling ‘void* memcpy(void*, const void*, size_t)’ with a pointer to a non-trivial type ‘struct NonTrivialButTriviallyCopyable’ [-Wnon-trivial-memaccess]
>    memcpy (dst, src, sizeof (*src));
>                                   ^
> $
>
> Implementations of vector-like classes can very well (and are
> encouraged) to make use of std::is_trivially_copyable to know whether
> they can copy a range of elements to new storage
> using memcpy/memmove/mempcpy.
>
> Running your patch against GDB trips on such a case:
>
> src/gdb/btrace.h: In function ‘btrace_insn_s* VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const btrace_insn_s*, const char*, unsigned int)’:
> src/gdb/common/vec.h:948:62: error: calling ‘void* memmove(void*, const void*, size_t)’ with a pointer to a non-trivial type ‘btrace_insn_s {aka struct btrace_insn}’ [-Werror=non-trivial-memaccess]
>    memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));    \
>                                                               ^
>
> There is nothing wrong with the code being warned here.
> While "struct btrace_insn" is trivial (has a user-provided default
> ctor), it is still trivially copyable.


Any news on getting a "fix" for this issue.  Right now it blocks my
testing of GCC/gdb because I am building the trunk of both in a CI
loop and my build is broken due to this warning.  Should I just add
--disable-werror to my gdb build instead?

Thanks,
Andrew Pinski

>
> Now, this gdb code is using the old VEC (originated from
> gcc's C days, it's not the current C++fied VEC implementation),
> but the point is that any other random vector-like container out there
> is free to optimize copy of a range of non-trivial but trivially
> copyable types using memcpy/memmove.
>
> Note that libstdc++ does not actually do that optimization, but
> that's just a missed optimization, see PR libstdc++/68350 [1]
> "std::uninitialized_copy overly restrictive for
> trivially_copyable types".  (libstdc++'s std::vector defers
> copy to std::unitialized_copy.)
>
> [1] - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350
>
>> These aren't undefined and the patch could be tweaked to allow
>> them.
>
> I think they're undefined because the types are not trivially
> copyable (non-trivial destructor with side effects).
>
>> I decided not to invest effort into it because, although
>> not strictly erroneous, I think they represent poor practice.
>
> I think that for my suggestion, all you really need is use
> call trivially_copyable_p instead of trivial_type_p, for
> memcpy/memmove/mempcpy at least.
>
> For memset, I'd suggest to go the other direction actually, and
> instead of relaxing the trivial check, to tighten it, by warning about
> memset'ting objects of non-standard-layout type too.  I.e., warn for
> memset of all non-POD (non-trivial + non-standard-layout) types.  That's
> what I did in the gdb patch.  That would also produce warnings for memset
> of trivial types that via refactoring end up with references, like:
>
> struct Trivial
> {
>   Trivial () = default;
>   Trivial (int &i) : m_i (i) {}
>   void add (int howmuch) { m_i += howmuch; }
>
> private:
>   int &m_i;
> };
>
> void reset (Trivial *triv)
> {
>   memset (triv, 0, sizeof (Trivial));
> }
>
> void recompute (Trivial *triv)
> {
>   reset (triv); // start over
>   triv->add (10); // whoops, null reference.
> }
>
> It's also warn for memset of trivial types that aren't standard
> layout due to having a mix of public/protected/private fields,
> which is likely not a real problem in practice, but I'd call
> those a code smell that warrants a warning too:
>
>  struct S
>  {
>  private:
>    int a;
>  public:
>    int b;
>  };
>
>  S s;
>  memset (&s, 0, sizeof (S));
>
>
> Playing with the patch, I noticed that you can't silence
> it by casting the pointer to void, but you can with
> casting to char, like:
>
> void copy (B *dst, B *src)
> {
>     memcpy (dst, src, sizeof (*src)); // warns
>     memcpy ((void*) dst, (void *) src, sizeof (*src)); // still warns
>     memcpy ((char*) dst, (char *) src, sizeof (*src)); // doesn't warn
>     memcpy ((unsigned char*) dst, (unsigned char *) src, sizeof (*src)); // doesn't warn
> }
>
> I can understand how we end up like that, given char's magic properties, but still
> I think many will reach for "void" first if they (really really) need to add a cast to
> silence the warning.  In any case, I think it'd be very nice to add cases with such
> casts to the new tests, and maybe mention it in the documentation too.
>
> Thanks,
> Pedro Alves
>