On Wed, Jun 17, 2020 at 6:31 PM Martin Sebor <mse...@gmail.com> wrote: > > On 6/17/20 1:13 AM, Richard Biener wrote: > > On Tue, Jun 16, 2020 at 9:35 PM Martin Sebor <mse...@gmail.com> wrote: > >> > >> On 6/16/20 3:33 AM, Richard Biener wrote: > >>> On Mon, Jun 15, 2020 at 7:11 PM Martin Sebor via Gcc-patches > >>> <gcc-patches@gcc.gnu.org> wrote: > >>>> > >>>> On 6/14/20 12:37 PM, Jeff Law wrote: > >>>>> On Sat, 2020-06-13 at 17:49 -0600, Martin Sebor wrote: > >>>>>> 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: > >>>>> Yes, this is the same issue we were discussing privately. > >>>>> > >>>>>> > >>>>>> 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). > >>>>> True, but the transformation done by PRE is valid. PRE is concerned > >>>>> only with > >>>>> value equivalences and the two addresses are the same and PRE can and > >>>>> will > >>>>> replace one with the other. > >>>> > >>>> That's fine. Since they are treated as equivalent it shouldn't > >>>> matter which of the equivalent alternatives is chosen (there > >>>> may be many). It's the particular choice of the smaller member > >>>> that makes it a problem: both in the terms of triggering a false > >>>> positive and in terms of the note referencing a member the source > >>>> code doesn't use. > >>>> > >>>> If PRE instead picked the bigger member it wouldn't necessarily > >>>> trigger the warning. But if that member was also too small, > >>>> the note might still reference the wrong member. > >>>> > >>>> But if PRE picked another equivalent representation not involving > >>>> any member at all but rather an offset from the base object (i.e., > >>>> just a MEM_REF) there would be no problem either way: no false > >>>> positive, and if it overflowed, the warning wouldn't reference > >>>> any member but just the base object. > >>>> > >>>>> > >>>>>> > >>>>>> 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: > >>>>> Ultimately that's just a bandaid over a flawed implementation. > >>>>> Fundamentally the > >>>>> problem is the diagnostics should not be depending on the type of those > >>>>> MEM > >>>>> expressions. As long as we continue to do that we're going to run into > >>>>> problems. > >>>>> Hence my suggestion we look at attaching suitable type information to > >>>>> the calls > >>>>> early in the pipeline, possibly at AST generation time. > >>>> > >>>> It's not the MEM_REF type that's a problem here. The general > >>>> issue with the reliability of late warnings like this one also > >>>> isn't isolated to just function calls. Any checked statement > >>>> is susceptible to them. > >>>> > >>>> In this case, the problem is that the COMPONENT_REF member is > >>>> not the one referenced in the source code. So more reliable > >>>> type information for MEM_REFs wouldn't solve it: we would also > >>>> need more reliable COMPONENT_REFs. > >>>> > >>>> I can think of a few ways to deal with it. > >>>> > >>>> 1) Introduce some alternate, more reliable, on-the-side > >>>> representation for warnings (your suggestion). > >>>> > >>>> 2) Accept that warnings that depend on these transforms are > >>>> inherently prone to false positives as a result (the status > >>>> quo). > >>>> > >>>> 3) Tighten up the requirements on the transforms/representations > >>>> to more faithfully correspond to the source code (my suggestion). > >>>> > >>>> Taken to its natural conclusion, (1) means we would need a whole > >>>> separate IL just for warnings. That doesn't seem sustainable. > >>>> > >>>> (2) is clearly suboptimal for users. Issuing some false positives > >>>> may be defensible but referencing code that doesn't even exist in > >>>> the source is wrong. > >>>> > >>>> Meeting the needs of both optimizations and warnings within the same > >>>> IL (i.e., (3)), seems like the right and only viable approach to me. > >>>> > >>>> For the case of this warning, when both of these > >>>> > >>>> &MEM[(struct sockaddr_in6 *)ai_10 + 4B].sin6_addr; > >>>> &MEM <struct sockaddr_in> [(void *)ai_10 + 4B].sin_zero; > >>>> > >>>> are treated as equivalent (because they evaluate to the same > >>>> address), is there some disadvantage or difficulty in encoding > >>>> them as > >>>> > >>>> &MEM <TYPE> [(void *)ai_10 + 4B + offsetof sin_zero]; > >>>> > >>>> instead? > >>> > >>> Well, we are _explicitely_ not doing this for the sake of diagnostics... > >> > >> Not doing the transformation I suggest at all, or not doing it in > >> cases when the member does match the source? > > > > We are currently not treating different fields at the same offset as > > "equal" before inlining completed for objsize to "work", thus leaving > > optimizations on the plate. > > It would be good to fix that at the same time so we can have both: > quality buffer overflow detection + diagnostics and optimal codegen. > > >> I'm suggesting to use the MEM_REF (offset) form only when the member > >> doesn't correspond to the source. > > > > But we don't know that - we only have the IL, not the source and for > > the case of PRE it doesn't know that it had two different representations > > because of the underlying algorithm. > > Just before FRE3 we have: > > _1 = &MEM[(struct A *)p_3(D) + 4B].a > _2 = &MEM[(struct B *)p_3(D) + 4B].b; > > (where a and b have the same offset). FRE3 then says: > > Replaced &MEM[(struct B *)p_3(D) + 4B].b with _1 in all uses of _2 = > &MEM[(struct B *)p_3(D) + 4B].b; > > What I suggest is to first replace _1 with > > MEM[(void *)p_3(D) + 8B] > > and then _2 with _1.
I suppose that would be possible. Not sure if desirable. > >>> > >>> It also falls apart if you have > >>> > >>> &MEM[].a[i_1] vs. &MEM[].b[i_1] > >>> > >>> do you want us to look for which array, a or b, is the bigger? > >> > >> If the expression in the source code is 'p->a[i]' and the IL ends > >> up with p->b[j] then, as I replied to Jeff, that's also a problem. > >> All the access warnings assume the object in an ARRAY_REF and > >> COMPONENT_REF (and MEM_REF) corresponds to what's in the source > >> code. > > > > That doesn't mean they are not wrong in doing so. Note you > > have to make a difference between addresses and actual > > memory accesses. Only for address computations the > > fields are nothing more than offsets. > > In the case of this bug the member-to-member transform is done for > an address computation whose result is used for an access by memset > and memcpy. But it's not done when the memcpy call is folded into > a MEM_REF assignment. Why this distinction when memcpy is folded > into MEM_REF all the time, both before and after FRE? The distinction is addresses vs. memory references. We usually want aggressive propagation to simplify addresses but we have to preserve access paths for memory accesses because they are used for alias-analysis where the actual path taken matters for correctness. > >>> But sure - specifically talking about PRE - we can simplify > >>> expressions inserted on the fly easily - we even pre-compute > >>> that offset and it is readily available. What we cannot > >>> do is do this _only_ when there are "conflicting" expressions > >>> (because we only know one of them at that point), we'd do > >>> it always. Look into create_component_ref_by_pieces_1 > >>> and the vn_reference_op_t's off member (see vn_reference_eq > >>> how it's used to only consider offsets). It will be a bit iffy > >>> to fold trailing recursive elements into the MEM_REF base > >>> but I guess by modifying the MEM_REF type and offset > >>> in-place instead of attaching a component with non -1 offset > >>> would be easiest. > >> > >> Thanks, I'll look into it. > >> > >>> > >>> You are suggesting to drop information (the access path that > >>> carries no semantics on GIMPLE). So you're fine with > >>> removing the crippling of FRE before inlining (which is > >>> there to preserve access paths to make __builtin_object_size > >>> "happy"?). > >> > >> I'm looking for feedback on the suggestion. I don't know all > >> the "tricks" in place to make __builtin_object_size (or other > >> things) happy. If it isn't viable or if there are better/easier > >> approaches I'd like to know before I spend time trying to get it > >> to work. > >> > >> For what it's worth, my hope is to ultimately make the following > >> possible: > >> > >> 1) Replace the _FORTIFY_SOURCE machinery with attribute access > >> (i.e., have calls to functions with attribute access emit > >> the same runtime checks as done in the __builtin____xxxcpy_chk() > >> functions. > > > > I think both will have exactly the same issue > > That's my concern. > > > but IMHO something > > like __builtin_object_size would be _easier_ to enhance because > > it funnels the "accesses" through another function. One idea that > > keeps popping up in my mind is change __builtin_object_size to > > receive not the address but a memory access as argument which > > can be massaged to also allow register typed accesses (contrary > > to what GIMPLE specifies right now). If those accesses are in > > call argument position there's no chance of CSE happening > > but regular SSA updating is still in effect. So you'd have > > > > sz_1 = __builtin_object_size (a.b[i_2].c, 1); > > ptr_3 = &a.b[i_2].c; // subject to "bogus" transforms > > memcpy_chk (ptr_3, ...., sz_1); > > > > where of course something close to the FE has to do the > > transformation on the __builtin_object_size. > > > > If you do everything with access attributes and get rid of > > the __builtin_object_size call you lose the "nice place" to > > stick the "unmodifiable" access tree. > > That sounds like a good approach for function calls. Where would > the __bos built-in call go for calls to user-defined functions > like > > void foo (int [static 7]); > > ? > > I suppose it would be an argument to a the artificial wrapper > emitted for each such call, analogous to __builtin___memcpy_chk: > > __foo_chk (ptr_3, 7, sz_1); > > that would either get stripped before expansion if the __bos > result was 7 * sizeof (int) or more, or replaced by the two > calls: > > __chk (ptr_3, sz_1); > foo (ptr_3); > > to do the checking at runtime (with _FORTIFY_SOURCE-like > checking). Does this sound reasonable? That sounds reasonable at first sight. > It doesn't solve the broader problem with false positives due > to the FRE/PRE transform, and from what you said above it doesn't > sound like changing PRE would be the right way to fix it (I'll > assume for now I'm still missing why.) Well, currently we have "heuristics" emitting diagnostics based on inherent "wrong" facts derived from the IL. Any fiddling with FRE or PRE does not change this fact but sure dropping information might reduce the number of false positives in exchange for more false negatives. > Could something similar to the above be done to avoid false > positives for non-call statement as well? I recall you were > concerned about __builtin_warning in the IL interfering with > optimizations and suggested using a debug statement. I haven't > yet looked into it and I have no idea how flexible this approach > might be. Any particular cases of non-call statements you refer to? Note the desire to push warnings later and later because more optimization creates new warning "opportunities" is in the end flawed - more optimizations will necessarily result in more false positives since most interesting optimizations do not preserve the original source structure. -fanalyzer to the rescue (well, maybe not, I must admit I did not look into its inner workings to tell whether it fulfills my suggestion of doing IPA static analysis before all optimizations). Richard. > Martin > > > > >> 2) Lay the foundation for a future implementation of C++ Contracts > >> (function attributes to express arbitrary constraints on function > >> arguments and return values checked at compile time if possible > >> and enforced at runtime). > >> > >> (2) is a generalization of (1). The primary purpose of both is > >> detecting bugs. The secondary purpose is enabling optimization. > >> So I'd like whatever I/we do now to be aligned with this bigger > >> goal (or at least not fundamentally incompatible with it). > >> > >> Martin > >> > >>> > >>> Mind you can't have both ;) > >>> > >>> Richard. > >>> > >>>> > >>>> Martin > >> >
