[PATCH] avoid false positives due to compute_objsize (PR 95353)

[Martin Sebor]

On 6/13/20 3:50 PM, Sandra Loosemore wrote:
> On 6/2/20 6:12 PM, Martin Sebor via Gcc-patches wrote:
>> The compute_objsize() function started out as a thin wrapper around
>> compute_builtin_object_size(), but over time developed its own
>> features to compensate for the other function's limitations (such
>> as its inability to work with ranges).  The interaction of these
>> features and the limitations has started to become increasingly
>> problematic as the former function is used in more contexts.
>>
>> A complete "fix" for all the problems (as well as some other
>> limitations) that I'm working on will be more extensive and won't
>> be appropriate for backports.  Until then, the attached patch
>> cleans up the extensions compute_objsize() has accumulated over
>> the years to avoid a class of false positives.
>>
>> To make the warnings issued based on the results of the function
>> easier to understand and fix, the patch also adds an informative
>> message to many instances of -Wstringop-overflow to point to
>> the object to which the warning refers.  This is especially
>> helpful when the object is referenced by a series of pointer
>> operations.
>>
>> Tested by boostrapping on x86_64-linux and building Binutils/GDB,
>> Glibc, and the Linux kernel with no new warnings.
>>
>> Besides applying it to trunk I'm looking to backport the fix to
>> GCC 10.
> 
> This patch (commit a2c2cee92e5defff9bf23d3b1184ee96e57e5fdd) has broken 
> glibc builds on nios2-linux-gnu, when building sysdeps/posix/getaddrinfo.c:
> 
> ../sysdeps/posix/getaddrinfo.c: In function 'gaih_inet.constprop':
> ../sysdeps/posix/getaddrinfo.c:1081:3: error: 'memcpy' writing 16 bytes 
> into a region of size 8 overflows the destination 
> [-Werror=stringop-overflow=]
>   1081 |   memcpy (&sin6p->sin6_addr,
>        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
>   1082 |    at2->addr, sizeof (struct in6_addr));
>        |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/netinet/in.h:3,
>                   from ../resolv/bits/types/res_state.h:5,
>                   from ../include/bits/types/res_state.h:1,
>                   from ../nptl/descr.h:35,
>                   from ../sysdeps/nios2/nptl/tls.h:45,
>                   from ../sysdeps/generic/libc-tsd.h:44,
>                   from ../include/../locale/localeinfo.h:224,
>                   from ../include/ctype.h:26,
>                   from ../sysdeps/posix/getaddrinfo.c:57:
> ../inet/netinet/in.h:249:19: note: destination object 'sin_zero'
>    249 |     unsigned char sin_zero[sizeof (struct sockaddr)
>        |                   ^~~~~~~~
> 
> 
> I have to say that I don't understand the "note" diagnostic here at all. 
>   :-(  Why does it think the destination object is a field of struct 
> sockaddr_in, while this memcpy is filling in a field of struct 
> sockaddr_in6?  (And, the sin6_addr field is indeed of type struct 
> in6_addr, matching the sizeof expression.)

Most likely because some earlier pass (from my exchange with Jeff
about this instance of the warning I suspect it's PRE) substitutes
one member for the other in the IL when offsets into them happen
to evaluate to the same offset from the start of the enclosing
object.  The Glibc code does this:

		struct sockaddr_in6 *sin6p =
		  (struct sockaddr_in6 *) ai->ai_addr;

		sin6p->sin6_port = st2->port;
		sin6p->sin6_flowinfo = 0;
		memcpy (&sin6p->sin6_addr,
			at2->addr, sizeof (struct in6_addr));

and the warning doesn't see sin6p->sin6_addr as the destination
but something like

   &MEM <struct sockaddr_in> [(void *)ai_10 + 4B].sin_zero;

The details in this and all other middle end warnings are only as
reliable as the IL they work with.  If the IL that doesn't correspond
to the original source code they're going to be confusing (and may
trigger false positives).

Instead of substituting one member for another in the COMPONENT_REF
when both happen to be accessed at the same offset, using a MEM_REF
alone into the enclosing struct or union plus the offset of
the members would avoid the problem.  Something like this:

   &MEM <char[]> [(void *)ai_10 + 4B + 8B];

(or whatever the final offset of the member is).

As for the warning itself, GCC tries to avoid warning for calls
to memcpy that write into multiple members at the same time, up
to the size of the complete object.  This is done because
the Linux kernel does these things in a few places.  As dangerous
as the practice is, the change I committed tries to preserve this
special exception for now (in the future I'd like to add a new
warning option to control it).  As I mentioned above I tested
the change with the kernel (as well as Glibc) and didn't see any
new warnings but it looks like this one slipped through (funny
that with all this testing it only triggers on such an uncommon
target).  I opened 95667 to fix it.  (The warning will most likely
come back once using memcpy to write across multiple members starts
getting diagnosed.)

Martin