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

[Martin Sebor]

On 05/16/2017 01:41 PM, Jason Merrill wrote:
> On Thu, May 11, 2017 at 12:23 PM, Martin Sebor <msebor@gmail.com> wrote:
>> The challenge with using memcpy or memset with class types is
>> figuring out if it's being called to copy-construct a new object
>> or assign a new value to an existing one.  In general it's not
>> possible to tell so the conservative assumption is that it's
>> the latter.
>>
>> Because of that, relying on the trivially copyable property to
>> determine whether it's safe to assign a new value to an object
>> is not sufficient (or even necessary).  The class must be at
>> a minimum trivially assignable.  But it turns out that even
>> trivial assignability as defined by C++ isn't enough.  A class
>> with a const data member or a member of a reference type is
>> considered "trivially assignable" but its copy assignment is
>> deleted, and so it can't be assigned to.   Calling memset or
>> memcpy to write over an object of such a class can violate
>> the const invariant or corrupt the reference, respectively.
>
> Yes, "trivially copyable" elides the differences between
> initialization and assignment context.  I agree that it makes sense to
> check for a trivial assignment operator specifically.  I guess we want
> a slightly stronger "trivially copyable" that also requires a
> non-deleted assignment operator.
>
> It seems to me that the relevant tests are:
>
> bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
> bzero/memset want trivial + non-deleted assignment.

I think that's very close to what the patch does.

bzero/memset: expects a trivial class with no private
members and with a non-deleted trivial assignment.  That looks
the same as what you have above.

bcopy/memcpy/memmove: expects the same plus trivially copyable,
except that the class may be non-trivial if the source of the
copy is any of void*, [signed or unsigned] char*, or a pointer
to the class itself.  In that case, the byte size must either
be non-constant or a multiple of the size of the object, to
help detect inadvertent partial copies.

The test for class triviality is to detect misusing the functions
to initialize (construct) an object of a class with a user-defined
default ctor by copying bytes into it from an object of unrelated
type (void* and char* are allowed for serialization).

I did forget about bcopy.  Let me add it.

>
> I'm still not convinced we need to consider standard-layout at all.

I agree.  The patch doesn't make use of is_standard_layout_p().
It defines its own helper called almost_std_layout_p() that
combines trivial_type_p() with tests for non-public members,
ignoring the requirement that all members of a derived standard
layout class must be defined in the same base (or derived) class.

Is there something you'd like to see done differently in the
latest revision of the patch?

https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00976.html

Martin