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

[Martin Sebor <msebor@gmail.com>]

On 05/16/2017 04:48 PM, Pedro Alves wrote:
> On 05/16/2017 08:41 PM, Jason Merrill wrote:
>
>> 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'm still not convinced we need to consider standard-layout at all.
>
> What do you think of warning for memset of types with references?  Since NULL
> references are undefined behavior (N4659, [dcl.ref]/5), IMHO such code is quite
> smelly and most likely a sign of incomplete refactoring, not design.
>
> (My original intention for poisoning memset of standard-layout type in gdb was
> mainly as proxy/approximation for "type with references".  Other properties
> that make a type non-standard-layout are not as interesting to me.)
>
> While at it, maybe the same reasoning would justify warn of memcpy/memset
> of types with const data members?  memcpy of such types kind of sounds like a
> recipe for subtle breakage that could only be salvaged with std::launder.
> And if you know about that, you'll probably be copying objects of type T
> to/from raw byte/char storage, not to/from another T.
>
> Actually, now that I look at Martin's new patch, I see he's already warning
> about const data members and references, both memcpy and memset.  I'm not
> super convinced on warning about memcpy of references (unlike memset), but
> I don't feel strongly against it either.  I'd be fine (FWIW) with giving it a
> try and see what breaks out there.

I did this because objects with references cannot be assigned
to (the default copy assignment is deleted).  So as a baseline
rule, I made the warning trigger whenever a native assignment
or copy isn't valid.  In the IMO unlikely event that a memcpy
over a reference is intended, the warning is easy to suppress.

I expect calling memset on an object with a reference to almost
certainly be a bug since there's no way to make such a reference
usable.  The only time it might be intentional is when someone
tries to wipe out the storage before deleting the object in
the storage (e.g., in a dtor).  But that's a misuse as well
because such calls are typically eliminated, much to many
a security analyst's shock and horror.  I'd like to eventually
diagnose that too (though possibly under a different warning).

I used a similar (though not exactly the same) rationale for
warning for const members.  They too cannot be assigned to,
and letting memset or memcpy silently change them violates
const-correctnes.  It's also undefined and the immutability
of such members an optimization opportunity waiting to be
exploited.

>> +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.
>
> May I suggest renaming the warning (and the description above) from
> -Wnon-trivial-memaccess to something else that avoids "trivial" in
> its name?  Because it's no longer strictly about "trivial" in the
> standard C++ sense.  The documentation talks about "The safe way",
> and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess?

I debated whether to rename things (not just the warning but
also the function that implements it in GCC).  Ultimately I
decided it wasn't necessary because the rules seem close enough
to be based on some notion of triviality and because no better
name came to mind. -Wunsafe-memaccess might work.  The one mild
concern I have with it is that it could suggest it might do more
than simple type checking (e.g., buffer overflow and what not).
Let's see what Jason thinks.

> (I spotted a couple typos in the new patch: "otherwse", "becase", btw.)

I'm a horrible typist.  I'll proofread the patch again and fix
them up before committing it.

Martin