[Bug driver/115293] Warn if a compiler flag downgrades protection provided by -fhardened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115293 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED --- Comment #3 from Siddhesh Poyarekar --- Invalid, since this is already implemented through -Whardened.
[Bug driver/115293] Warn if a compiler flag downgrades protection provided by -fhardened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115293 --- Comment #2 from Siddhesh Poyarekar --- Oh, I had overlooked -Whardened; so it looks like we already do this.
[Bug driver/115293] Warn if a compiler flag downgrades protection provided by -fhardened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115293 Siddhesh Poyarekar changed: What|Removed |Added Version|13.0|14.0 CC||mpolacek at gcc dot gnu.org Severity|normal |enhancement
[Bug driver/115293] New: Warn if a compiler flag downgrades protection provided by -fhardened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115293 Bug ID: 115293 Summary: Warn if a compiler flag downgrades protection provided by -fhardened Product: gcc Version: 13.0 Status: UNCONFIRMED Severity: normal Priority: P3 Component: driver Assignee: unassigned at gcc dot gnu.org Reporter: siddhesh at gcc dot gnu.org Target Milestone: --- When -fhardened is passed alongside options it enables, the options could override behaviour enabled by -fhardened. This is by design, but there's a hole in this, in that accidentally passing, e.g. -fstack-protector alongside -fhardened could downgrade stack protection. Add a new warning (-Wweakened-hardening) that points out such a situation, allowing developers to turn off the warning if the downgrade in hardening is deliberate.
[Bug tree-optimization/99475] [11 Regression] bogus -Warray-bounds accessing an array element of empty structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99475 Siddhesh Poyarekar changed: What|Removed |Added CC||siddhesh at gcc dot gnu.org --- Comment #7 from Siddhesh Poyarekar --- This doesn't appear to be reproducible on trunk anymore, should we close it?
[Bug middle-end/113514] Imprecise __builtin_dynamic_object_size when using a set local variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113514 Siddhesh Poyarekar changed: What|Removed |Added Last reconfirmed||2024-01-22 Ever confirmed|0 |1 Status|UNCONFIRMED |NEW
[Bug middle-end/113514] Wrong __builtin_dynamic_object_size when using a set local variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113514 --- Comment #5 from Siddhesh Poyarekar --- What seems to be happening is that early_objsz bails out since the subobject size at that point is not a constant; I remember concluding that it's safest to stick to constant sizes here, but I can't remember why I came to that conclusion. Then in constant propagation (literally the next pass in -O2), the reference gets folded into a MEM_REF and we have the classic case of the subobject reference being lost, due to which we see the whole object size there instead of the subobject size. I need to try and remember why I decided against generating expressions in early_objsz.
[Bug tree-optimization/113012] [13 regression] ICE when building xorg-server with -fsanitize=undefined
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113012 Siddhesh Poyarekar changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #13 from Siddhesh Poyarekar --- ... and now fixed on the 13 branch too.
[Bug tree-optimization/113012] [13 regression] ICE when building xorg-server with -fsanitize=undefined
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113012 --- Comment #11 from Siddhesh Poyarekar --- Yes, I'll test and push the 13 backport by the end of the week if there are no reported regressions on trunk.
[Bug tree-optimization/113013] [12/13/14 regression] ICE in fold_convert_loc with -fsanitize=undefined
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113013 --- Comment #6 from Siddhesh Poyarekar --- Sorry I misread the reproducer as void *reallocarray(void) __attribute__((__alloc_size__(1))); Your fix looks fine to me, thanks.
[Bug tree-optimization/113013] [12/13/14 regression] ICE in fold_convert_loc with -fsanitize=undefined
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113013 Siddhesh Poyarekar changed: What|Removed |Added Keywords|ice-on-valid-code |ice-on-invalid-code CC||siddhesh at gcc dot gnu.org --- Comment #4 from Siddhesh Poyarekar --- Agreed, the attribute is invalid and maybe the frontend needs to flag it early and flag an error.
[Bug ipa/96503] attribute alloc_size effect lost after inlining
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 --- Comment #8 from Siddhesh Poyarekar --- (In reply to Martin Uecker from comment #7) > For __builtin_with_access we probably only want to allow > reducing the object size, while the 'extend_size' workaround > used by systemd (cf comment #4) would need to extend it. > Maybe we need another flag? I've been thinking of a new __object_size__ attribute to annotate functions that behave like __builtin_object_size so that calls to it override (and not just reduce) sizes returned by allocations. I can then use it to annotate a supported malloc_usable_size replacement (say, malloc_size_estimate) that actually works like __builtin_object_size and can then be used by systemd.
[Bug ipa/96503] attribute alloc_size effect lost after inlining
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 --- Comment #6 from Siddhesh Poyarekar --- So basically, __builtin_with_access(void *ptr, size_t size, int access) where access == -1: Unknown access semantics 0: none 1: read_only 2: write_only 3: read_write should address both access and alloc_size and even counted_by. We would need to emit the builtin in the caller as well as callee of the function that has the access attribute while for alloc_size, we only need to emit this in the caller.
[Bug ipa/96503] attribute alloc_size effect lost after inlining
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 --- Comment #5 from Siddhesh Poyarekar --- This could work for alloc_size, but not quite for access. pointer_with_size (or __builtin_with_size as you suggested in that thread) would need to express access semantics too, to be able to express everything that access does.
[Bug testsuite/110763] FAIL: gcc.dg/ubsan/object-size-dyn.c -O2 execution test
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110763 Siddhesh Poyarekar changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED --- Comment #2 from Siddhesh Poyarekar --- Fixed.
[Bug tree-optimization/110373] New: __builtin_object_size does not recognize subarrays in multi-dimensional arrays
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110373
Bug ID: 110373
Summary: __builtin_object_size does not recognize subarrays in
multi-dimensional arrays
Product: gcc
Version: 13.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: tree-optimization
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
const char example[][32] = {
"abcd",
"xyzt",
"1234",
"5678"
};
__SIZE_TYPE__ foo (void) {
return __builtin_dynamic_object_size(example[1], 1);
}
$ gcc/cc1 -quiet -O -o - ~/src/random/fortify-module/fortify-module1.c
.file "fortify-module1.c"
.text
.globl foo
.type foo, @function
foo:
.LFB0:
.cfi_startproc
movl$96, %eax
ret
.cfi_endproc
.LFE0:
.size foo, .-foo
.globl example
.section.rodata
.align 32
.type example, @object
.size example, 128
example:
.string "abcd"
.zero 27
.string "xyzt"
.zero 27
.string "1234"
.zero 27
.string "5678"
.zero 27
.ident "GCC: (GNU) 14.0.0 20230622 (experimental)"
.section.note.GNU-stack,"",@progbits
Clang returns 32 instead, recognizing the subarray correctly. This appears to
be happening very early on in the frontend; addr_object_size doesn't see that
this is a subarray reference since it only compares the pt_var with the operand
of the ADDR_EXPR. If pt_var is an ARRAY_REF, it should also additionally see
if this is a subarray reference.
[Bug middle-end/109557] __builtin_dynamic_object_size() does not work for simple testing case
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557 Siddhesh Poyarekar changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |INVALID --- Comment #6 from Siddhesh Poyarekar --- (In reply to Martin Uecker from comment #5) > With "in general" I meant in a different program. From just knowing the > type of the pointer you can not derive the object size. This is how I > understood the original question. Ah ok, agreed. Closing this as invalid then; I noticed I was missing the inline keyword when I tried forcing inlining, so that was also a PEBKAC.
[Bug middle-end/109557] __builtin_dynamic_object_size() does not work for simple testing case
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557 --- Comment #4 from Siddhesh Poyarekar --- (In reply to Martin Uecker from comment #3) > I general the pointer could point to the first object of an array that has > more elements, or to an object of a different type. How so? p in comment 0 is just a NULL-initialized pointer. It gets assigned to a malloc'd storage in store() (which the code in main() cannot see) but until then, it's a NULL pointer.
[Bug middle-end/109557] __builtin_dynamic_object_size() does not work for simple testing case
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557 --- Comment #2 from Siddhesh Poyarekar --- (In reply to qinzhao from comment #0) > I am wondering for > p.3_1 = p; > _2 = __builtin_object_size (p.3_1, 0); > > why the size of p.3_1 cannot use the TYPE_SIZE of the pointee of p when its > size can be determined (i.e, not a structure with a flexible array member, > etc)? To answer this specific question, it's because the compiler can't see in main() if p is pointing to any actual storage.
[Bug middle-end/109557] __builtin_dynamic_object_size() does not work for simple testing case
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557 --- Comment #1 from Siddhesh Poyarekar --- The __bdos call itself cannot succeed in main() because it cannot see the allocation in store(). One way it could succeed is if store() was inlined, but for some reason it doesn't, even if you make the function static inline. If I decorate store() with __attribute__((inline)) I get the warning: foo.c:10:1: warning: ‘always_inline’ function might not be inlinable [-Wattributes] but it seems to proceed to inline the call because the assert in main() is no longer hit. So from the __bdos context I'm inclined to say NOTABUG.
[Bug tree-optimization/109334] tree-object-size: Improve size computation in arguments
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109334 --- Comment #2 from Siddhesh Poyarekar --- That seems OK; I had added that to be conservative since I really only intended to add support for the access attribute back then and not the implicit attributes. Could you please post that on the ML and have it reviewed? Thanks!
[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104970 --- Comment #14 from Siddhesh Poyarekar --- (In reply to Martin Uecker from comment #13) > This fix seem too radical. It now prevents this from working even when there > is an explicit attribute but there is also a VLA bound. Also I think it > would be nice if it worked without the explicit attribute at least in the > simple cases where this should be now problem. I've cloned this to bug 109334.
[Bug tree-optimization/109334] New: tree-object-size: Improve size computation in arguments
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109334
Bug ID: 109334
Summary: tree-object-size: Improve size computation in
arguments
Product: gcc
Version: 13.0
Status: UNCONFIRMED
Keywords: ice-on-valid-code
Severity: normal
Priority: P3
Component: tree-optimization
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
CC: jakub at gcc dot gnu.org, marxin at gcc dot gnu.org,
msebor at gcc dot gnu.org, muecker at gwdg dot de,
siddhesh at gcc dot gnu.org
Depends on: 104970
Target Milestone: ---
The fix for bug 104970 is too restrictive, size computation should also work
for VLA bounds and also cases where it could work without attributes in simple
cases, e.g.
size_t
__attribute__ ((noinline))
test_parmsz_internal3 (size_t sz1, size_t sz2, double obj[sz1][sz2])
{
return __builtin_dynamic_object_size (obj, 0);
}
or
__attribute__ ((noinline, access (read_only, 2, 1)))
int foo(int n, int buf[n])
{
buf[n] = 1;
return __builtin_dynamic_object_size(buf, 0);
}
int main() {
int n = 10;
int buf[n];
return foo(n, buf);
}
Referenced Bugs:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104970
[Bug 104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since
r12-6480-gea19c8f33a3a8d2b
[Bug sanitizer/109308] False positive store to address 0x62600000016c with insufficient space for an object of type 'int' since r12-6030-g422f9eb7011b76c1
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109308 --- Comment #5 from Siddhesh Poyarekar --- This kinda has happened before: https://github.com/Perl/perl5/issues/20678 Should we keep this bug open for the message, which is obviously wrong?
[Bug libgcc/109270] ssp/ssp.h should be adapted to use __builtin_dynamic_object_size()
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109270 Siddhesh Poyarekar changed: What|Removed |Added Ever confirmed|0 |1 Last reconfirmed||2023-03-24 Status|UNCONFIRMED |ASSIGNED Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org
[Bug c/108896] provide "element_count" attribute to give more context to __builtin_dynamic_object_size() and -fsanitize=bounds
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108896 --- Comment #22 from Siddhesh Poyarekar --- (In reply to Martin Uecker from comment #20) > > I haven't seen comments on Kees's first example, where "malloc" returns an > > "__alloc_size" hint that's lost when "p" is returned from the function (at > > least in Clang). If that information is kept around, it would expand the > > number > > of bounds checks we could perform. (Kudos if this is currently the case in > > GCC.) > > I think not. But this would not work across TU boundaries anyway. > > https://godbolt.org/z/eW9GT579r It's not that the information is lost in this case, it is just not visible, unlike bug 96503 for example where the information is actually lost. The example above doesn't work because the compiler is instructed to *not* look inside foo; I'd expect something like LTO to work even across TU boundaries whenever possible. There's the external linkage gap, but I don't know if that's a relevant motivation for the kernel. >From the kernel (or distribution builds) perspective, __element_count__ will cover situations where it's not straightforward to propagate sizes, e.g. where the target code is an allocator that itself wants to enforce size constraints on objects it returns, beyond the input size request. This really should have been the way __access__ was implemented, but we tied that attribute to only functions. Would it be a terrible idea to make __element_count__ more general purpose so that it ends up deprecating __access__?
[Bug tree-optimization/108522] [12 Regression] ICE in tree-object-size when struct has VLA
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108522 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #7 from Siddhesh Poyarekar --- Fixed on master as well as gcc-12 now.
[Bug tree-optimization/107952] tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952 --- Comment #16 from Siddhesh Poyarekar --- (In reply to Qing Zhao from comment #15) > Since S2.flex is not an “array_ref”, it’s correct for > array_ref_fleixble_size_p to return false for it, I think. > We might add a new utility routine to determine whether a ref to a struct or > union have flexible array? It will be useful for __bos/__bdos for sure.
[Bug tree-optimization/107952] tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952 --- Comment #14 from Siddhesh Poyarekar --- (In reply to Qing Zhao from comment #13) > > > > The first is handled by the function just fine, > > No, even the first case is not recognized by the current > “array_ref_flexible_size_p”, it’s not been identified as a flexible array > right now. > Shall we include this case into “array_ref_flexible_size_p”? (It’s a GCC > extension). In the first case, array_ref_flexible_size_p recognizes S2.flex.data as having flexible size. The tests in my patch[1] for this bug checks for this. However, array_ref_flexible_size_p does not recognize S2.flex as having flexible size. It might make sense to support that, i.e. any struct or union with the last element as a flex array should be recognized as having flexible size. [1] https://sourceware.org/pipermail/gcc-patches/2022-December/608912.html
[Bug tree-optimization/107952] tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952
--- Comment #12 from Siddhesh Poyarekar ---
(In reply to qinzhao from comment #7)
> (In reply to Richard Biener from comment #1)
> > GCC considered this as a flex-array.
>
> do you mean for the following example:
>
> typedef struct {
> char pad;
> char data[];
> } F2;
>
> typedef struct {
> unsigned pad;
> F2 flex;
> } S2;
>
> although C standard disallow the above, GCC extension treats S2.flex.data as
> a flex-array?
>
> How about:
>
> typedef struct {
> char pad;
> char data[];
> } F2;
>
> typedef struct {
> F2 flex;
> unsigned pad;
> } S2;
>
> do we have any documentation on this Gcc extension?
There's an open bug to document these semantics:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77650(In reply to
rguent...@suse.de from comment #11)
> On Tue, 24 Jan 2023, qing.zhao at oracle dot com wrote:
>
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952
> >
> > --- Comment #10 from Qing Zhao ---
> > > --- Comment #9 from Richard Biener ---
> > >
> > > GCC handles for example
> > >
> > > struct A { char data[1]; };
> > > struct B { int n; struct A a; };
> > >
> > > as if the a.data[] array is a flex-array.
> >
> > Okay. Then the maximum size of __builtin_object_size for it should be -1,
> > right?
>
> I think so.
Why? If the a B object is allocated with a visible allocator call, we can
return the correct size here too.
[Bug tree-optimization/108522] [Regression 12/13] ICE in tree-object-size when struct has VLA
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108522 Siddhesh Poyarekar changed: What|Removed |Added See Also||https://bugzilla.redhat.com ||/show_bug.cgi?id=2164035 Ever confirmed|0 |1 Last reconfirmed||2023-01-24 Keywords||ice-on-valid-code Status|UNCONFIRMED |ASSIGNED Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org
[Bug tree-optimization/108522] New: [Regression 12/13] ICE in tree-object-size when struct has VLA
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108522
Bug ID: 108522
Summary: [Regression 12/13] ICE in tree-object-size when struct
has VLA
Product: gcc
Version: 13.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: tree-optimization
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
Reproducer:
__SIZE_TYPE__ foo (unsigned int n)
{
struct {
unsigned char a[n];
unsigned char b;
} s;
return __builtin_dynamic_object_size (, 1);
}
$ gcc/cc1 -quiet ../repro.cc -O2 -Wall -o -
.file "repro.cc"
.text
during GIMPLE pass: objsz
../repro.cc: In function ‘foo’:
../repro.cc:1:15: internal compiler error: in execute_todo, at passes.cc:2140
1 | __SIZE_TYPE__ foo (unsigned int n)
| ^~~
0x72216f execute_todo
../../gcc/passes.cc:2140
Please submit a full bug report, with preprocessed source (by using
-freport-bug).
Please include the complete backtrace with any bug report.
See
[Bug tree-optimization/107952] tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952
--- Comment #8 from Siddhesh Poyarekar ---
(In reply to qinzhao from comment #7)
> (In reply to Richard Biener from comment #1)
> > GCC considered this as a flex-array.
>
> do you mean for the following example:
>
> typedef struct {
> char pad;
> char data[];
> } F2;
>
> typedef struct {
> unsigned pad;
> F2 flex;
> } S2;
>
> although C standard disallow the above, GCC extension treats S2.flex.data as
> a flex-array?
>
> How about:
>
> typedef struct {
> char pad;
> char data[];
> } F2;
>
> typedef struct {
> F2 flex;
> unsigned pad;
> } S2;
>
> do we have any documentation on this Gcc extension?
There's an open bug to document these semantics:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77650
[Bug tree-optimization/108398] tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108398 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED --- Comment #9 from Siddhesh Poyarekar --- Original code also had the same UB, which I've sent a PR to fix: https://github.com/dk/Prima/pull/79
[Bug tree-optimization/108398] tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108398 --- Comment #7 from Siddhesh Poyarekar --- Thanks, is that from the code in prima[1] or the Red Hat bugzilla report? The latter is undefined as per the above discussion. [1] https://github.com/dk/Prima/issues/78
[Bug tree-optimization/108398] tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108398
--- Comment #5 from Siddhesh Poyarekar ---
Ack, I had a thinko with
unsigned steps[] = {1, 1};
because in that case too n_steps doesn't get decremented, resulting in OOB
access. I'm going to look at the original report[1] to see if the test case
reduction was valid and will close this out as invalid if there's nothing
interesting for the compiler to do there. Thanks!
[Bug tree-optimization/108398] tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108398
--- Comment #3 from Siddhesh Poyarekar ---
Oops, sorry I messed up the reproducer, here's the correct one. The principles
don't really change though:
unsigned steps[2];
int main(void) {
unsigned n_steps = sizeof (steps) / sizeof (unsigned);
for (unsigned *io = steps; 0 < n_steps; io++) {
if (*io == 0) {
__builtin_printf ("%zu\n", __builtin_dynamic_object_size (io, 0));
if (__builtin_dynamic_object_size (io, 0) < sizeof (unsigned))
__builtin_abort ();
n_steps--;
io--;
}
}
return 0;
}
[Bug tree-optimization/108398] tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108398 --- Comment #2 from Siddhesh Poyarekar --- Yeah, I've been ping-ponging about the validity too, which is why I filed a bug to get some consensus position. I suppose if we don't treat it as a bug, should we try and support it in cases we can by attempting some heuristics, like we'd like to do for invalidated realloc input pointers? https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105217
[Bug tree-optimization/108398] New: tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108398
Bug ID: 108398
Summary: tree-object-size trips up with pointer arithmetic if
an intermediate result is an invalid pointer
Product: gcc
Version: 13.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: tree-optimization
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
Reproducer:
unsigned steps[2];
int main(void) {
for (unsigned *io = steps; 0 < sizeof (steps) / sizeof (unsigned); io++) {
if (*io == 0) {
if (__builtin_dynamic_object_size (io, 0) != sizeof (unsigned))
__builtin_abort ();
io--;
}
}
return 0;
}
$ gcc -O1 prima.c -o prima
$ ./prima
Aborted (core dumped)
io may momentarily point before steps, which is what seems to trip up
tree-object-size.
[Bug tree-optimization/105043] Documentation for __builtin_object_size and other Object Size checking builtin functions should mention - D_FORTIFY_SOURCE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105043 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #10 from Siddhesh Poyarekar --- The glibc complement of this documentation is here: https://patchwork.sourceware.org/project/glibc/patch/20221222160403.4151387-1-siddh...@sourceware.org/
[Bug tree-optimization/105043] Documentation for __builtin_object_size and other Object Size checking builtin functions should mention - D_FORTIFY_SOURCE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105043 Siddhesh Poyarekar changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org See Also||https://sourceware.org/bugz ||illa/show_bug.cgi?id=28998 --- Comment #8 from Siddhesh Poyarekar --- I'm working on some documentation in glibc. I agree the last part of the Object Size Checking documentation can be rephrased in the context of _FORTIFY_SOURCE, let me take a shot at that.
[Bug tree-optimization/107952] tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952
--- Comment #5 from Siddhesh Poyarekar ---
(In reply to rguent...@suse.de from comment #4)
> Does it allow the nesting when nested in a union?
data[] cannot be nested directly in a union (i.e. union { char d; char data[];
} is invalid) but something like below is allowed, again as a gcc extension,
not by the standard. The standard AFAICT doesn't allow anything other than the
flex array directly at the end of the top level struct.
typedef struct {
unsigned pad;
union {
struct {char d; char data[];} f1;
struct {char d; char data[];} f2;
} flex;
} S2;
In fact this is the use case that led me into this rabbithole.
[Bug tree-optimization/107952] tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952 --- Comment #2 from Siddhesh Poyarekar --- The standard does not allow the nesting, but gcc supports it as an extension. The middle end does see the array as a flex array correctly, but tree-object-size doesn't seem to do the right thing consistently enough. Or rather, the current behaviour seems a bit mixed up, which is why I want to try and specify the behaviour for tree-object-size more clearly with all combinations of -fstrict-flex-array and arrays, i.e. nested or otherwise.
[Bug c/107951] Invalid flexible array use not detected in nested structs by the C frontend
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107951 Siddhesh Poyarekar changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |DUPLICATE --- Comment #5 from Siddhesh Poyarekar --- (In reply to Andrew Pinski from comment #3) > r0-44662-g2984fe64968ad7 added that documentation. > PR 15749 shows that at one point there was code floating around (glibc?) > that uses the extension (_G_iconv_t). Yeah but it doesn't address bug 77650, which I agree that this is a duplicate of. Thanks for pointing out! *** This bug has been marked as a duplicate of bug 77650 ***
[Bug c/77650] struct with a nested flexible array followed by another member accepted
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77650 Siddhesh Poyarekar changed: What|Removed |Added CC||siddhesh at gcc dot gnu.org --- Comment #6 from Siddhesh Poyarekar --- *** Bug 107951 has been marked as a duplicate of this bug. ***
[Bug tree-optimization/107952] New: tree-object-size: inconsistent size for flexible arrays nested in structs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107952
Bug ID: 107952
Summary: tree-object-size: inconsistent size for flexible
arrays nested in structs
Product: gcc
Version: 13.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: tree-optimization
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
With -fstrict-flex-arrays, there is a clearer definition of what constitutes
flexible array members, controlled by the frontend. tree-object-size however
doesn't seem to handle this well enough when the flex array is nested. e.g.:
typedef struct {
char pad;
char data[8];
} F2;
typedef struct {
unsigned pad;
F2 flex;
} S2;
#define NULL (void *) 0
__SIZE_TYPE__
nested_flexarray (__SIZE_TYPE__ n)
{
S2 *p = (S2 *) __builtin_malloc (n);
return __builtin_dynamic_object_size (p->flex.data, 1);
}
__SIZE_TYPE__
nested_flexarray2 (S2 *p)
{
return __builtin_dynamic_object_size (p->flex.data, 1);
}
The current behaviour is to treat data as a flex array and nested_flexarray
returns the size as if it were a flex array. nested_flexarray2 however returns
the subscripted size, thinking of it as a fixed array, which should not be the
expected behaviour with -fstrict-flex-arrays=0. Instead it should return -1
for maximum size and perhaps 8 as minimum size.
If F2 is changed to:
typedef struct {
char pad;
char data[0];
} F2;
the current behaviour ends up returning 0 in both cases, which is incorrect.
Again, it should return the size as if it were a flex array in nested_flexarray
and -1 for maximum size for nested_flexarray2.
I have a patch that does this, but I need to fix up tests that currently expect
older behaviour and wanted to get some consensus before I fixed them up.
[Bug c/107951] New: Invalid flexible array use not detected in nested structs by the C frontend
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107951
Bug ID: 107951
Summary: Invalid flexible array use not detected in nested
structs by the C frontend
Product: gcc
Version: unknown
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: c
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
The following program:
typedef struct {
char pad;
char data[];
} F2;
typedef struct {
F2 flex;
unsigned pad;
} S2;
#define NULL (void *) 0
__SIZE_TYPE__
nested_flexarray (__SIZE_TYPE__ n)
{
S2 *p = __builtin_malloc (n);
return __builtin_dynamic_object_size (p->flex.data, 1);
}
ends up treating data[] as a zero sized array in C instead of flagging an
error. This is correctly handled in the C++ driver.
[Bug tree-optimization/107038] Bogus -Wstringop-overflow in dead code
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107038 Siddhesh Poyarekar changed: What|Removed |Added Last reconfirmed||2022-10-07 Version|13.0|12.0 Status|UNCONFIRMED |NEW Ever confirmed|0 |1
[Bug tree-optimization/107038] Bogus -Wstringop-overflow in dead code
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107038 --- Comment #8 from Siddhesh Poyarekar --- I forgot to mention that I've been building with: gcc/cc1 -o /dev/null ../bogus-stringop-overflow.i -O2 -Werror=stringop-overflow -quiet to reproduce the warning: ../bogus-stringop-overflow.i: In function ‘elf_begin_rand’: ../bogus-stringop-overflow.i:35:19: error: ‘foo_alias’ writing 14 or more bytes into a region of size 10 overflows the destination [-Werror=stringop-overflow=] 35 | ret = foo_alias (buf, nbytes, offset); | ^~~ ../bogus-stringop-overflow.i:14:8: note: destination object ‘ar_size’ of size 10 14 | char ar_size[10]; |^~~ ../bogus-stringop-overflow.i:8:16: note: in a call to function ‘foo_alias’ declared with attribute ‘access (write_only, 1, 2)’ 8 | extern ssize_t foo_alias (void *buf, size_t nbytes, off_t offset) |^ cc1: some warnings being treated as errors
[Bug tree-optimization/107038] Bogus -Wstringop-overflow in dead code
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107038
Siddhesh Poyarekar changed:
What|Removed |Added
Summary|[13 Regression] Bogus |Bogus -Wstringop-overflow
|-Wstringop-overflow= since |in dead code
|r13-2789-gb40b3035879cf695 |
--- Comment #7 from Siddhesh Poyarekar ---
[Removing the commit ref and regression since this has been known to fail for
elfutils with __bdos]
OK so here's a reduced reproducer that ought to avoid undefined behaviour. It
is in fact a case of warning on unreachable code:
typedef long int off_t;
typedef long int ssize_t;
typedef long unsigned int size_t;
extern ssize_t foo_chk (void *buf, size_t nbytes, off_t offset, size_t sz)
__attribute__((__access__(__write_only__, 1, 2)));
extern ssize_t foo_alias (void *buf, size_t nbytes, off_t offset)
__attribute__((__access__(__write_only__, 1, 2)));
struct ar_hdr
{
int buf;
char ar_size[10];
};
int
elf_begin_rand(void)
{
struct ar_hdr h = {.ar_size = {0}};
size_t len = sizeof(h.ar_size);
ssize_t recvd = 0;
do
{
ssize_t ret;
do
{
char *buf = h.ar_size + recvd;
size_t nbytes = len - recvd;
off_t offset = recvd + __builtin_offsetof (struct ar_hdr, ar_size);
size_t bdos = __builtin_dynamic_object_size (buf, 0);
if (__builtin_constant_p (bdos) && bdos == (size_t) -1)
ret = foo_alias (buf, nbytes, offset);
else
ret = foo_chk (buf, nbytes, offset, bdos);
}
while (ret < 0);
recvd += ret;
}
while ((size_t) recvd < len);
return recvd;
}
So what's happening here is that ranger tries to infer the ranges backwards
from the possibly taken branch foo_alias to start from the fact that bdos ==
-1. Here's the IR snippet:
[local count: 1073741824]:
# recvd_6 = PHI
recvd.0_1 = (sizetype) recvd_6;
_25 = recvd.0_1 + 4;
_26 = MAX_EXPR <_25, 16>;
_27 = _26 - recvd.0_1;
_24 = _27 + 18446744073709551612;
buf_13 = _size + recvd.0_1;
nbytes_14 = 10 - recvd.0_1;
_3 = recvd.0_1 + 4;
offset_15 = (off_t) _3;
bdos_16 = _24;
Working backwards, _24 is seen to be +INF like bdos_16, which gives _27 the
range of [3,3].
Given that _26 has range of [16, +INF], recvd.0_1 ends up with a range of [13,
18446744073709551612], leaving nbytes with a range of [14,
18446744073709551613].
Ideally somewhere in that chain there ought to have been some hint to indicate
that one of those ranges is impossible, but there isn't. The nbytes range for
example ought to be limited to [1-10]. Initializing ret in the above program
allows ranger to see that correct range.
[Bug tree-optimization/107038] [13 Regression] Bogus -Wstringop-overflow= since r13-2789-gb40b3035879cf695
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107038 --- Comment #5 from Siddhesh Poyarekar --- (In reply to Siddhesh Poyarekar from comment #4) > (In reply to Martin Liška from comment #2) > > > I assume this is elfutils #29614? > > > > Yes. > > > > Please take a look at the original unreduced testcase I attached here. > > That looks like unpatched elfutils. I know you mentioned[1] that you're > using the latest elfutils but can you please confirm again? Or maybe > incorrect preprocessed file? > > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=29614#c2 OK maybe that's irrelevant from the gcc perspective because this is basically warning on unreachable code, which we know tends to happen sometimes. The comment is mainly for the elfutils issue you're facing.
[Bug tree-optimization/107038] [13 Regression] Bogus -Wstringop-overflow= since r13-2789-gb40b3035879cf695
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107038 --- Comment #4 from Siddhesh Poyarekar --- (In reply to Martin Liška from comment #2) > > I assume this is elfutils #29614? > > Yes. > > Please take a look at the original unreduced testcase I attached here. That looks like unpatched elfutils. I know you mentioned[1] that you're using the latest elfutils but can you please confirm again? Or maybe incorrect preprocessed file? [1] https://sourceware.org/bugzilla/show_bug.cgi?id=29614#c2
[Bug tree-optimization/107038] [13 Regression] Bogus -Wstringop-overflow= since r13-2789-gb40b3035879cf695
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107038 --- Comment #1 from Siddhesh Poyarekar --- recvd is uninitialized and it seems to be preventing optimization of the fortify macro one way or for some reason. I can take a look at why the condition does not get folded away but a reproducer without undefined behaviour may be more persuasive. I assume this is elfutils #29614?
[Bug tree-optimization/105736] [12/13 Regression] ICE in force_gimple_operand_1, at gimplify-me.cc:79 since r13-222-g28896b38fabce818
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105736 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #6 from Siddhesh Poyarekar --- Fixed for gcc 13 and 12.
[Bug tree-optimization/97185] inconsistent builtin elimination for impossible range
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97185 --- Comment #3 from Siddhesh Poyarekar --- (In reply to Martin Sebor from comment #2) > There's a heuristic for ranges of allocation sizes to exclude zero > (size_range_flags) that comes into play here. The actual range isn't > "impossible" in the sense it's necessarily invalid. It just means the > string function call is either a no-op or out of bounds, and so can be > eliminated as an optimization. With the optimization consistently > implemented the warning will also go away (eliminating the calls will > prevent sanitizers from detecting the out of bounds ones, so that might be a > consideration). Ahh I see it now, I had missed that it was an 'int'. ISTM that a better idea would be to *not* optimize away memcpy and memmove in this case, not the other way around. > > In general, a low > high range denoted an anti-range before Ranger was > introduced (i.e., ~[high, low]). With Ranger it's the corresponding union > of two ranges. Some of the cruft for dealing with anti-ranges is still > around, such as in get_size_range() in pointer-query.cc. The code should be > migrated to the irange class and the representation probably also updated to > print something more sensible (e.g., the union [MIN, high) U (low, MAX]; we > talked about introducing a pretty-printer % directive for ranges to make the > format consistent across all diagnostics). That makes sense, thanks.
[Bug middle-end/101836] __builtin_object_size(P->M, 1) where M is an array and the last member of a struct fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 --- Comment #26 from Siddhesh Poyarekar --- (In reply to qinzhao from comment #25) > So, based on all the discussion so far, how about the following: > > ** add the following gcc option: > > -fstrict-flex-arrays=[0|1|2|3] > > when -fstrict-flex-arrays=0: > treat all trailing arrays as flexible arrays. the default behavior; Wouldn't this be -fno-strict-flex-arrays, i.e. the current behaviour? > when -fstrict-flex-arrays=1: > Only treating [], [0], and [1] as flexible array; > > when -fstrict-flex-arrays=2: > Only treating [] and [0] as flexible array; > > when -fstrict-flex-arrays=3: > Only treating [] as flexible array; The strictest level. If yes, then you end up having: -fstrict-flex-arrays=[1|2|3] with, I suppose, 1 as the default based on Jakub's comment about maximum compatibility support.
[Bug tree-optimization/97185] inconsistent builtin elimination for impossible range
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97185 --- Comment #1 from Siddhesh Poyarekar --- While the missed optimization ought to be fixed, what's the value of -Wstringop-* warning on an impossible range, i.e. when low > high? Shouldn't it just bail out silently if it detects an impossible range?
[Bug tree-optimization/105736] [12/13 Regression] ICE in force_gimple_operand_1, at gimplify-me.cc:79 since r13-222-g28896b38fabce818
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105736
--- Comment #3 from Siddhesh Poyarekar ---
Here we go, I'll put it into builtin-dynamic-object-size-0.c, bootstrap and
post a patch.
struct TV4
{
__attribute__((vector_size (sizeof (int) * 4))) int v;
};
struct TV4 val3;
int *
f1 (struct TV4 *a)
{
return >v[0];
}
int
f2 (void)
{
int *t = f1 ();
if (__builtin_dynamic_object_size (t, 0) != -1)
__builtin_abort ();
return 0;
}
[Bug middle-end/101836] __builtin_object_size(P->M, 1) where M is an array and the last member of a struct fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
--- Comment #23 from Siddhesh Poyarekar ---
(In reply to Siddhesh Poyarekar from comment #22)
> An arbitrary N will only make it abuse-friendly and potentially mask bugs.
> IMO if we choose to make multiple levels here it should only be
> -fstrict-flex-arrays={1,2} where 1 (the default) only allows "[]" and 2
> allows "[0]", disabling all other size values. For anything else,
That could be ""[0]" or "[1]", disabling all other size values" if we want to
build gcc and vim with -fstrict-flex-arrays and keep fortification enabled.
Vim explicitly disables fortification right now for this reason.
> -fno-strict-flex-arrays. My opinion on the default is not strong FWIW.
Also I wonder if there should be an analogous -Wstrict-flex-arrays to issue
warnings alongside changing codegen.
[Bug middle-end/101836] __builtin_object_size(P->M, 1) where M is an array and the last member of a struct fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
--- Comment #22 from Siddhesh Poyarekar ---
(In reply to Kees Cook from comment #21)
> How about "-fnot-flex-arrays=N" to mean "trailing arrays with N or more
> elements will NOT be treated like a flex array"?
>
> Then code with sockaddr can use "-fnot-flex-arrays=15", code with "[1]"
> arrays can use "-fnot-flex-arrays=2", code with only "[0]" arrays can use
> "-fnot-flex-arrays=1", and "-fstrict-flex-arrays" can be an alias for
> "-fnot-flex-arrays=0", which Linux would use.
An arbitrary N will only make it abuse-friendly and potentially mask bugs. IMO
if we choose to make multiple levels here it should only be
-fstrict-flex-arrays={1,2} where 1 (the default) only allows "[]" and 2 allows
"[0]", disabling all other size values. For anything else,
-fno-strict-flex-arrays. My opinion on the default is not strong FWIW.
[Bug tree-optimization/105736] [12 Regression] ICE in force_gimple_operand_1, at gimplify-me.cc:79 since r13-222-g28896b38fabce818
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105736
--- Comment #2 from Siddhesh Poyarekar ---
OK, so the fix is pretty straightforward; error_mark_node escapes through as a
return in ADDR_EXPR object size computations. I want to get a reproducer
independent of ubsan though so that it's verifiable in the general case:
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index fc062b94d76..f1a699a94db 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -695,19 +695,24 @@ addr_object_size (struct object_size_info *osi,
const_tree ptr,
var_size = pt_var_size;
bytes = compute_object_offset (TREE_OPERAND (ptr, 0), var);
if (bytes != error_mark_node)
- bytes = size_for_offset (var_size, bytes);
- if (var != pt_var
- && pt_var_size
- && TREE_CODE (pt_var) == MEM_REF
- && bytes != error_mark_node)
{
- tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0), pt_var);
- if (bytes2 != error_mark_node)
+ bytes = size_for_offset (var_size, bytes);
+ if (var != pt_var
+ && pt_var_size
+ && TREE_CODE (pt_var) == MEM_REF
+ && bytes != error_mark_node)
{
- bytes2 = size_for_offset (pt_var_size, bytes2);
- bytes = size_binop (MIN_EXPR, bytes, bytes2);
+ tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0),
+ pt_var);
+ if (bytes2 != error_mark_node)
+ {
+ bytes2 = size_for_offset (pt_var_size, bytes2);
+ bytes = size_binop (MIN_EXPR, bytes, bytes2);
+ }
}
}
+ else
+ bytes = size_unknown (object_size_type);
wholebytes
= object_size_type & OST_SUBOBJECT ? var_size : pt_var_wholesize;
[Bug middle-end/101836] __builtin_object_size(P->M, 1) where M is an array and the last member of a struct fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 --- Comment #7 from Siddhesh Poyarekar --- I couldn't work on -fstrict-flex-arrays then, sorry. I do have it in my plan for gcc 13, but I'll admit it's not on the very top of my list of things to do this year. If you or anyone else needs a stronger guarantee of this making it into gcc 13 and wants to work on it, I'll be happy to help with reviews.
[Bug tree-optimization/105736] [12 Regression] ICE in force_gimple_operand_1, at gimplify-me.cc:79 since r13-222-g28896b38fabce818
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105736 Siddhesh Poyarekar changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org Summary|[13 Regression] ICE in |[12 Regression] ICE in |force_gimple_operand_1, at |force_gimple_operand_1, at |gimplify-me.cc:79 since |gimplify-me.cc:79 since |r13-222-g28896b38fabce818 |r13-222-g28896b38fabce818 Version|13.0|12.0 Keywords||ice-on-valid-code --- Comment #1 from Siddhesh Poyarekar --- Also affects gcc 12; it's only that it's more easily visible through the ubsan generated __bdos call like in this reproducer. The valid size check is missing a check to ensure that it's not an error_mark_node.
[Bug middle-end/105709] FORTIFY_SOURCE=3 (*** buffer overflow detected ***: terminated) on Qt
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105709
--- Comment #9 from Siddhesh Poyarekar ---
>From a quick check of non-reduced-qt.cxx, clang appears to fail to fortify the
readlink function, which may explain why you see the failure with gcc but not
clang. Also the reduced reproducer in comment 1 looks wrong; it passes a 0
object size to __readlink_chk, which is guaranteed to fail. The correct
reproducer in that context is:
```
extern "C" void __readlink_chk(char *, char *, long, long);
char readlink___path, readlink___buf;
namespace Qt {
enum Initialization {} Uninitialized;
}
struct QArrayData {
int size;
};
struct QByteArray {
QByteArray(int, Qt::Initialization);
~QByteArray();
int size() const;
QArrayData d;
};
QByteArray::~QByteArray() {}
int QByteArray::size() const { return d.size; }
main() {
QByteArray buf(6, Qt::Uninitialized);
int __trans_tmp_1 = buf.size();
__readlink_chk(___path, ___buf, __trans_tmp_1,
__builtin_dynamic_object_size (___buf, 0));
}
```
which again, is invalid code because the readlink is passed a 1 byte buffer and
read an uninitialized number of bytes, which again fails correctly. Fun fact:
this code will likely *pass* if -ftrivial-auto-var-init is passed! I guess one
can't win everything...
Now looking at the original code, it seems similar to the issue in bug 105078,
which is basically an attempt to use an implicit flex array (by overallocating
memory to the object) which is not guaranteed to work all the time. Clang
simply bails out at some point, because of which it doesn't fortify the
readlink call and everything is good.
[Bug middle-end/105566] [13 regression] ICE in gfortran.dg/ubsan/bind-c-intent-out-2.f90 after r13-222-g28896b38fabce8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105566 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED --- Comment #1 from Siddhesh Poyarekar --- I pushed a fix for this moments ago: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70090#c8
[Bug middle-end/70090] add non-constant variant of __builtin_object_size for _FORTIFY_SOURCE and -fsanitize=object-size
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70090 Siddhesh Poyarekar changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #9 from Siddhesh Poyarekar --- __builtin_dynamic_object_size is in gcc12 and ubsan now has dynamic object size support in gcc13. Fixed thusly.
[Bug tree-optimization/105217] Likely wrong code with -D_FORTIFY_SOURCE=3 since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105217 --- Comment #5 from Siddhesh Poyarekar --- (In reply to Jakub Jelinek from comment #4) > Then there is the case where we can clearly see that the pointer from malloc > is passed to realloc or can trace it to such easily. I'd say in that case > it would be worthwhile to do some extra work. > For __bos the simplest solution would be if we detect something like that > (e.g. that the SSA_NAME passed to realloc has uses dominated by the realloc > call (though, even figuring that can mean we e.g. mark gimple stmts in each > bb with increasing uids to determine like reassoc what stmt is before > another one) just to punt, say we don't know anything about the SSA_NAME's > size, or use conservative choice from both malloc and realloc (maximum for > bos0/bos1, minimum for bos2/bos3). > For __bdos perhaps the same. Another possibility would be to temporarily > split the SSA_NAME passed to realloc, kind like old VRP was introducing > ASSERT_EXPRs. > So, basically when we see: > whatever = realloc (p_34, ...); > rewrite that (temporarily?) to: > p_121 = p_34; > whatever = realloc (p_121, ...); > and change all p_34 uses dominated by the realloc stmt to p_121, and add the > p_121 = p_34; stmt to some hash table or otherwise mark it so that we > wouldn't propagate the objsz knowledge from p_34 to p_121, but instead set > it on the realloc call. That won't cover the integral comparisons though > I'm afraid... This sounds like a gcc 13+ project. Can we downgrade this since the reproducer is technically invalid and we're only going to attempt to support a limited subset of such uses?
[Bug tree-optimization/105217] Likely wrong code with -D_FORTIFY_SOURCE=3 since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105217
Siddhesh Poyarekar changed:
What|Removed |Added
CC||jakub at gcc dot gnu.org
--- Comment #3 from Siddhesh Poyarekar ---
Here's a simplified version that shows the problem:
typedef __SIZE_TYPE__ size_t;
#define OLD 40
#define NEW 80
#define OFF 10
int
main (void)
{
char *p = __builtin_malloc (OLD);
char *q = 0;
char *dst = p + OFF;
q = __builtin_realloc (p, NEW);
if (q == 0)
__builtin_unreachable ();
if (q != p)
{
p = q;
dst = q + OFF;
}
__builtin_printf ("old size: %zu, new size: %zu, __bdos: %zu\n",
OLD - OFF, NEW - OFF,
__builtin_dynamic_object_size (dst, 0));
}
The problem is when realloc does not result in a different buffer (q == p);
`dst` is left untouched assuming that it's the same object. I doubt if this is
a portable assumption, since the C standard says that value of q (and
consequently dst?) will have become invalid beyond its lifetime, i.e. the
moment it is realloc'd:
6.2.4 Storage durations of objects
...
The value of a pointer becomes indeterminate when the object it points to (or
just past) reaches the end of its lifetime.
...
It just happens so that with the glibc malloc implementation it remains valid
but ISTM that applications should not rely on it.
Jakub, would you concur with this?
[Bug tree-optimization/105217] Likely wrong code with -D_FORTIFY_SOURCE=3 since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105217 --- Comment #2 from Siddhesh Poyarekar --- OK, taking a closer look, it looks like clang simply fails to fortify fread (probably due to https://reviews.llvm.org/D109967 or something similar). Modifying the code to use __fread_chk directly: size_t rdct = __fread_chk (data, __builtin_dynamic_object_size (data, 0), (size_t)1, rem_sz, fp); causes clang to crash too because it too comes up with the same __bdos estimate for size: ``` fread: data=0xf792c0 (dsize: 16344, size: 18446744073709551615), rem_sz=16340 .. read rdct=16340 realloc to=0xf7e490-0xf83489 (newsize=20473) .. diferent buffer! fread: data=0xf82484 (dsize: 4101, size: 18446744073709551615), rem_sz=4096 .. read rdct=4096 realloc to=0xf7e490-0xf84489 (newsize=24569) fread: data=0xf83484 (dsize: 5, size: 18446744073709551615), rem_sz=4096 *** buffer overflow detected ***: terminated Aborted (core dumped) ``` dsize and size are the actual values that __bdos and __bos resolve to; I simply modified the fprintf to this: fprintf(stderr, "fread: data=%p (dsize: %zu, size: %zu), rem_sz=%d\n", data, __builtin_dynamic_object_size (data, 0), __builtin_object_size (data, 0), rem_sz); I haven't looked too closely at the failure mechanism (I will tomorrow), but this has got me inclined to think that it's an actual autogen bug that got exposed with _FORTIFY_SOURCE=3.
[Bug tree-optimization/105217] Likely wrong code with -D_FORTIFY_SOURCE=3
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105217 Siddhesh Poyarekar changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org Status|NEW |ASSIGNED
[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078 Siddhesh Poyarekar changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |WONTFIX --- Comment #7 from Siddhesh Poyarekar --- OK then, this one is a WONTFIX too, the libQt6 version of the function should reliably produce the intended effect qtbase needs.
[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078 --- Comment #5 from Siddhesh Poyarekar --- (In reply to Martin Liška from comment #4) > Note the libQt6 version of the function looking approximately like this: > This doesn't warn anymore (and doesn't crash either) because objsz cannot get past the bit_and_expr: gimple_assign gimple_assign gimple_assign gimple_call <__builtin_object_size, _2, start_13, 1> However, ISTM from the remaining IR that thanks to the casts, it will see the right size if it got past the bit_and_expr. Jakub, should we recognize bit_and_expr in objsz for gcc13?
[Bug tree-optimization/105078] Maybe wrong *** buffer overflow detected ***: terminated with -D_FORTIFY_SOURCE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105078
--- Comment #1 from Siddhesh Poyarekar ---
With gcc12:
Computing maximum subobject size for _11:
Visiting use-def links for _11
Visiting use-def links for _10
Computing maximum object size for header_12:
Visiting use-def links for header_12
header_12: maximum object size 272
_10: maximum subobject size 16
_11: maximum subobject size 0
Simplified
_2 = __builtin_object_size (_11, 1);
to 0
gimple_simplified to if (0 != 0)
gimple_simplified to _4 = 0;
int main ()
{
struct QArrayData * header;
long unsigned int _2;
int _3;
bool _4;
int _5;
long int iftmp.1_6;
long int iftmp.2_7;
long int iftmp.2_8;
long int iftmp.1_9;
struct QArrayData * _10;
void * _11;
[local count: 1073741824]:
header_12 = malloc (272);
header_12->size = 256;
header_12->offset = 16;
_10 = [(struct QTypedArrayData *)header_12].D.4557;
_11 = _10 + 16;
_2 = __builtin_object_size (_11, 1);
_4 = 0;
_5 = __builtin_constant_p (_4);
if (_5 != 0)
goto ; [50.00%]
else
goto ; [50.00%]
...
with gcc11:
;; Function main (main, funcdef_no=54, decl_uid=4523, cgraph_uid=48,
symbol_order=47) (executed once)
Computing maximum subobject size for _11:
Visiting use-def links for _11
Visiting use-def links for header_12
_11: maximum subobject size 256
header_12: maximum subobject size 272
Simplified
_2 = __builtin_object_size (_11, 1);
to 256
gimple_simplified to if (0 != 0)
gimple_simplified to if (1 != 0)
gimple_simplified to _4 = 1;
int main ()
{
struct QArrayData * header;
long unsigned int _2;
int _3;
bool _4;
int _5;
long int iftmp.1_6;
long int iftmp.2_7;
long int iftmp.2_8;
long int iftmp.1_9;
void * _11;
[local count: 1073741823]:
header_12 = malloc (272);
header_12->size = 256;
header_12->offset = 16;
_11 = [(void *)header_12 + 16B];
_2 = __builtin_object_size (_11, 1);
_4 = 1;
_5 = __builtin_constant_p (_4);
if (_5 != 0)
goto ; [50.00%]
else
goto ; [50.00%]
...
The
[(void *)header_12 + 16B]
vs
_10 = [(struct QTypedArrayData *)header_12].D.4557;
_11 = _10 + 16;
appears to be the difference, where the gcc11 version allows the full size
(272) to be seen while the cast to QTypedArrayData in the latter allows only
the sizeof (QTypedArrayData) to be seen as subobject size.
[Bug tree-optimization/104964] Wrong *** buffer overflow detected ***: terminated - acl
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104964
Siddhesh Poyarekar changed:
What|Removed |Added
Status|ASSIGNED|RESOLVED
Resolution|--- |WONTFIX
--- Comment #15 from Siddhesh Poyarekar ---
(In reply to Jakub Jelinek from comment #14)
> Thus I'd say fix up acl instead.
OK, closing this as WONTFIX then. __bos/__bdos has limited support for zero
sized arrays; they are not recognized as flex arrays when in nested structs.
Fixing up the struct to one with a proper flex array (i.e. without a dimension
size, which also will need another member preceding it) should make this work
correctly. Something like:
struct __string_ext
{
char pad;
char s_str[];
};
[Bug tree-optimization/104964] Wrong *** buffer overflow detected ***: terminated - acl
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104964
--- Comment #13 from Siddhesh Poyarekar ---
It's not really a regression AFAICT, it's only more visible with __bdos because
non-constant offsets don't stop it. Also the problem is only with subobjects
(hence limited to _FORTIFY_SOURCE > 1 for strcpy) where the block in
addr_object_size that is supposed to deal with flex arrays at the end doesn't
quite do its job with nested structs.
The same reproducer tweaked a bit will crash even for __builtin_object_size:
struct __string_ext
{
char s_str[0];
};
typedef struct
{
int o_prefix;
struct __string_ext i;
} string_obj;
#define SUFFIX ".suffix"
string_obj *
__acl_to_any_text (unsigned long n)
{
unsigned long off = 0;
unsigned long size = sizeof SUFFIX;
string_obj *obj = __builtin_malloc (sizeof (string_obj) + size);
if (n == 0)
__builtin_unreachable ();
while (n-- != 0)
{
if (off + 1 > size - sizeof SUFFIX)
{
size <<= 1;
string_obj *tmp = __builtin_realloc (obj, sizeof (string_obj) +
size);
if (!tmp)
__builtin_unreachable ();
obj = tmp;
}
obj->i.s_str[off++] = 'A';
}
char *t = obj->i.s_str;
__strcpy_chk (t, SUFFIX, __builtin_object_size (t, 1));
return obj;
}
int
main ()
{
string_obj *s = __acl_to_any_text (32);
__builtin_printf ("%zu: %s\n", __builtin_strlen (s->i.s_str), s->i.s_str);
return 0;
}
$ gcc/cc1 -g -o test.s -quiet -Wall -O3 fs3.c
fs3.c: In function ‘__acl_to_any_text’:
fs3.c:40:3: warning: ‘__builtin___memcpy_chk’ writing 8 bytes into a region of
size 0 overflows the destination [-Wstringop-overflow=]
40 | __strcpy_chk (t, SUFFIX, __builtin_object_size (t, 1));
| ^~
[Bug tree-optimization/104964] Wrong *** buffer overflow detected ***: terminated - acl
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104964 --- Comment #11 from Siddhesh Poyarekar --- (In reply to Siddhesh Poyarekar from comment #10) > OK, I have a representative reproducer, which TBH is not too different from > the one you posted, just that it succeeds with __builtin_object_size and > fails with __builtin_dynamic_object_size: *crashes* with __builtin_dynamic_object_size and doesn't with __builtin_object_size. Sorry, I realized I used "fails" and "succeeds" in two opposite and confusing contexts there ;)
[Bug tree-optimization/104964] Wrong *** buffer overflow detected ***: terminated - acl
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104964
Siddhesh Poyarekar changed:
What|Removed |Added
Status|NEW |ASSIGNED
--- Comment #10 from Siddhesh Poyarekar ---
OK, I have a representative reproducer, which TBH is not too different from the
one you posted, just that it succeeds with __builtin_object_size and fails with
__builtin_dynamic_object_size:
struct __string_ext
{
char s_str[0];
};
typedef struct
{
int o_prefix;
struct __string_ext i;
} string_obj;
#define SUFFIX ".suffix"
string_obj *
__acl_to_any_text (unsigned long n)
{
unsigned long off = 0;
unsigned long size = sizeof SUFFIX;
string_obj *obj = __builtin_malloc (sizeof (string_obj) + size);
if (n == 0)
__builtin_unreachable ();
while (n-- != 0)
{
if (off + 1 > size - sizeof SUFFIX)
{
size <<= 1;
string_obj *tmp = __builtin_realloc (obj, sizeof (string_obj) +
size);
if (!tmp)
__builtin_unreachable ();
obj = tmp;
}
obj->i.s_str[off++] = 'A';
}
char *t = obj->i.s_str + off;
__strcpy_chk (t, SUFFIX, __builtin_dynamic_object_size (t, 1));
return obj;
}
int
main ()
{
string_obj *s = __acl_to_any_text (32);
__builtin_printf ("%zu: %s\n", __builtin_strlen (s->i.s_str), s->i.s_str);
return 0;
}
$ gcc/cc1 -g -o test.s -quiet -Wall -O3 fs3.c
fs3.c: In function ‘__acl_to_any_text’:
fs3.c:40:3: warning: ‘__builtin___memcpy_chk’ writing 8 bytes into a region of
size 0 overflows the destination [-Wstringop-overflow=]
40 | __strcpy_chk (t, SUFFIX, __builtin_dynamic_object_size (t, 1));
| ^~
The only reason why __builtin_object_size fails is because of the non-constant
OFF. If that is removed, __builtin_object_size also returns the declared size
of s_str, i.e. 0. The check for a traditionally declared trailing array ()i.e.
a[0] or a[1]) seems to be broken for nested structs like the above. Change
that to s_str[] (the struct then needs another member above) and it works fine.
[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104970 Siddhesh Poyarekar changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #12 from Siddhesh Poyarekar --- Fixed.
[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104970
--- Comment #8 from Siddhesh Poyarekar ---
(In reply to Martin Sebor from comment #7)
> The dollar sign in the internal attr_access string implies a VLA bound and
> the attr_access::vla_bounds() function queries the VLA bounds. That should
> make it possible to distinguish the two cases.
I noticed a attr_access.internal_p which has the following comment in
attribs.c:
/* Forms containing the square bracket are internal-only
(not specified by an attribute declaration), and used
for various forms of array and VLA parameters. */
That should be a good differentiator right? It fixes the motivating case here.
This is what I'm testing a bit more extensively.
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index b0b50774936..1e87739eda6 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -1477,7 +1477,7 @@ parm_object_size (struct object_size_info *osi, tree var)
tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm)));
tree sz = NULL_TREE;
- if (access && access->sizarg != UINT_MAX)
+ if (access && access->sizarg != UINT_MAX && !access->internal_p)
{
tree fnargs = DECL_ARGUMENTS (fndecl);
tree arg = NULL_TREE;
[Bug tree-optimization/104969] Likely a false positive of -D_FORTIFY_SOURCE=3
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104969 Siddhesh Poyarekar changed: What|Removed |Added See Also||https://sourceware.org/bugz ||illa/show_bug.cgi?id=28989 Resolution|--- |INVALID Status|NEW |RESOLVED --- Comment #4 from Siddhesh Poyarekar --- Moving this to glibc, since gcc is working as expected.
[Bug tree-optimization/104970] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104970 Siddhesh Poyarekar changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org Status|NEW |ASSIGNED --- Comment #1 from Siddhesh Poyarekar --- Looks like a codegen issue with parm_object_size; it's referring to a non-existent SSA name. I'll take a look at it first thing on Tuesday.
[Bug tree-optimization/104969] Likely a false positive of -D_FORTIFY_SOURCE=3
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104969 --- Comment #2 from Siddhesh Poyarekar --- (In reply to Martin Liška from comment #0) > The original code is defective a bit as it wrongly assumes that > (char*)str + (2 * i) is at maximum 'len' big. It's actually len - (2 * i) > big. But it should be still valid code, am I right? It doesn't overflow in this case, but specifying a length larger than the actual buffer size is a standard violation. """ The snprintf() function shall be equivalent to sprintf(), with the addition of the n argument which states the size of the buffer referred to by s. If n is zero, nothing shall be written and s may be a null pointer. Otherwise, output bytes beyond the n-1st shall be discarded instead of being written to the array, and a null byte is written at the end of the bytes actually written into the array. """ https://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html
[Bug tree-optimization/104964] Wrong *** buffer overflow detected ***: terminated - acl
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104964
Siddhesh Poyarekar changed:
What|Removed |Added
Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot
gnu.org
--- Comment #5 from Siddhesh Poyarekar ---
I'm not 100% sure if it's invalid code, but I was just about to write that it
depends on what the pass ends up seeing. If earlier passes end up optimizing
the code such that the objsz pass sees the malloc first (e.g. the reproducer in
pr104961), it ends up with the malloc'd size, otherwise it ends up with the
declared size.
So if it was:
struct bad_struct {
struct g
{
char s_str[1];
} i;
};
and
struct g *i = >i;
strcpy (i->s_str, "sparta");
then i tends to get optimized as a MEM_REF of the malloc'd block, letting us
see the extra space.
This needs to be fixed, but then it's possibly a different bug from the one
you're seeing in acl since this affects __bos too, not just __bdos.
(I'm off in a couple of hours btw, returning on Tuesday so I may not get to it
until then)
[Bug tree-optimization/104941] [12 Regression] ICE error: invalid (pointer) operands ‘minus_expr’ since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104941 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #3 from Siddhesh Poyarekar --- Fixed.
[Bug tree-optimization/104942] [12 Regression] ICE in size_for_offset, at tree-object-size.cc:352 since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104942 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #2 from Siddhesh Poyarekar --- Fixed.
[Bug tree-optimization/104941] ICE error: invalid (pointer) operands ‘minus_expr’ since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104941 Siddhesh Poyarekar changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org
[Bug tree-optimization/104942] ICE in size_for_offset, at tree-object-size.cc:352 since r12-6482-g06bc1b0c539e3a60
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104942 Siddhesh Poyarekar changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org Status|NEW |ASSIGNED
[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104854 --- Comment #8 from Siddhesh Poyarekar --- (In reply to Martin Sebor from comment #7) > Moving warnings into the analyzer and scaling it up to be able to run by > default, during development, sounds like a good long-term plan. Until that That's not quite what I'm suggesting here. I'm not a 100% convinced that those are the right heuristics at all; the size argument for strnlen, strndup and strncmp does not intend to describe the size of the passed strings. It is only recommended security practice that the *n* variant functions be used instead of their unconstrained relatives to mitigate overflows. In fact in more common cases the size argument (especially in case of strnlen and strncmp) may describe a completely different buffer or some other application-specific property. This is different from the -Wformat-overflow, where there is a clear relationship between buffer, the format string and the string representation of input numbers and we're only tweaking is the optimism level of the warnings. So it is not just a question of levels of verosity/paranoia. In that context, using size to describe the underlying buffer of the source only makes sense only for a subset of uses, making this heuristic quite noisy. So what I'm actually saying is: the heuristic is too noisy but if we insist on keeping it, it makes sense as an analyzer warning where the user *chooses* to look for pessimistic scenarios and is more tolerant of noisy heuristics. > happens, rather than gratuitously removing warnings that we've added over > the years, just because they fall short of the ideal 100% efficacy (as has > been known and documented), making them easier to control seems like a > better approach. It's not just a matter of efficacy here IMO. The heuristic for strnlen, strncmp and strndup overreads is too loose for it to be taken seriously.
[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104854 --- Comment #6 from Siddhesh Poyarekar --- (In reply to Martin Sebor from comment #5) > It would be useful to separate these warnings into multiple levels: level 1 > for invalid code, and higher levels for suspicious (or pointless) code, > similarly to -Wformat-overflow. I think the analyzer is a great level for the higher level heuristics, with ME warnings only sticking to level 1. Adding levels within ME warnings seems unnecessary. ISTM that users tend to *expect* false positives (to some sane extent) when doing static analysis but are much less tolerant of those during usual builds.
[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104854
Siddhesh Poyarekar changed:
What|Removed |Added
Summary|-Wstringop-overread should |-Wstringop-overread should
|not warn for strnlen and|not warn for strnlen,
|strndup |strndup and strncmp
Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot
gnu.org
--- Comment #4 from Siddhesh Poyarekar ---
(In reply to Martin Sebor from comment #3)
> The same warning is also issued for some calls to memchr(), strncmp() and
> probably other built-in functions as well. For example:
>
> const char a[] = "123";
>
> char* f (int i)
> {
> return __builtin_memchr (a + i, '3', 7);
> }
>
> warning: ‘__builtin_memchr’ specified bound 7 exceeds source size 4
> [-Wstringop-overread]
> 5 | return __builtin_memchr (a + i, '3', 7);
> | ^~~~
> y.c:1:12: note: source object allocated here
> 1 | const char a[] = "123";
> |^
The warning is arguably legitimate (not this one of course, since it's evident
at compile time that the operation is safe but for non-const `a` it may not be)
for memchr because it operates on an object, not string and will traverse all
of the object for a matching char to the extent of the object size. The
*string* functions are not the same in that context.
> In all these cases the warnings are intentional (i.e., it's a feature, and
> so not a regression). Their purpose is to point out what could be a coding
> mistake. With strndup(), since the use case for it rather than strdup() is
> to copy an initial substring, specifying a bound that's larger than the
> length of the string is pointless.
>
> In general, the GCC manual documents warnings as
>
> diagnostic messages that report constructions that are not inherently
> erroneous but that are risky or suggest there may have been an error.
I don't think these strnlen or strndup warnings point to *risky* or potentially
erroneous code; at best they point to instances where a specific protection is
absent, i.e. the behaviour reduces to strlen and strdup respectively, which is
much more benign than what it currently suggests. That kind of warning seems
more suited to a static analyzer and not a compiler IMO.
Besides, the core cause of a potential overread here is not because the size
argument is larger but because the string may not be NULL terminated. It may
make more sense to invest in that aspect of risky behaviour than the length for
these functions.
> So not all instances of any warning should be expected to indicate errors.
> In fact, many are known to be benign by design (for example, all instances
> of -Wchar-subscripts are benign when -funsigned-char is used; many instance
> of -Waddress are also benign, as are many -Wparentheses, etc.). And
They're not clubbed in with potentially important warnings though, which is a
key distinction. For example, one could mark -Wstirngop-overread as important
warnings but not -Wparentheses, but the high noise could make it difficult to
actually do so.
> although most middle end warnings tend to be designed to trigger only for
> invalid statements (i.e., statements that have undefined behavior if reached
> during execution), some do also trigger for code that's strictly valid but
> suspect. Besides the -Wstringop-overread examples here, other such warnings
> include dynamic allocation calls with sizes in excess of PTRDIFF_MAX
> (-Walloc-size-larger-than), returning the address of a local variable
> (-Wreturn-local-addr), or in GCC 12, storing the address of a local variable
> in an extern pointer (-Wdangling-pointer).
>
> Deciding what code is suspect enough to warn and under what option (-Wall or
> -Wextra) is a judgment call; different people have very different ideas.
I'm testing a patch to resolve this. I noticed strncmp late but it seems to
match this category as well, where a too-long length only reduces the max
protection provided by the n-versions of the string functions.
[Bug middle-end/104854] [11/12 Regression] -Wstringop-overread should not warn for strnlen and strndup
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104854
--- Comment #2 from Siddhesh Poyarekar ---
(In reply to David Malcolm from comment #1)
> Compiler Explorer link for the above (with -fanalyzer -Wall
> -Wstringop-overread -O2; -O2 seems to be needed to trigger it):
Ah yes, sorry, I pasted an older reproducer. This:
char *
foo (void)
{
return __builtin_strndup ("test", 8);
}
doesn't need -O2.
$ gcc -S -Wall ../test.c
../test.c: In function ‘foo’:
../test.c:5:10: warning: ‘__builtin_strndup’ specified bound 8 exceeds source
size 5 [-Wstringop-overread]
5 | return __builtin_strndup ("test", 8);
| ^
[Bug middle-end/104854] New: [11 Regression] -Wstringop-overread should not warn for strnlen and strndup
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104854
Bug ID: 104854
Summary: [11 Regression] -Wstringop-overread should not warn
for strnlen and strndup
Product: gcc
Version: 11.2.1
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: middle-end
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
The only times that strnlen and strndup can result in an actual overread is:
- The source is zero sized
or
- The source is not NULL terminated
In the current state something as trivial as the following:
char *
foo (size_t size)
{
return __builtin_strndup ("test", size);
}
char *
bar (void)
{
return foo (20);
}
issues a warning when the code is harmless. This is probably better suited as
a static analysis heuristic/suggestion than as a compiler warning that tends to
suggest that something is more likely wrong than not.
[Bug tree-optimization/104009] [12 Regression] r12-6030-g422f9eb7011b76c1 breaks kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104009 Siddhesh Poyarekar changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #4 from Siddhesh Poyarekar --- I built a `make allyesconfig` kernel with this patch and trunk as of yesterday, so fixed.
[Bug tree-optimization/104009] r12-6030-g422f9eb7011b76c1 breaks kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104009 Siddhesh Poyarekar changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org Priority|P3 |P1 CC||marxin at gcc dot gnu.org Last reconfirmed||2022-01-13 Ever confirmed|0 |1 Status|UNCONFIRMED |ASSIGNED --- Comment #1 from Siddhesh Poyarekar --- Bumping to P1 since we want to be able to build the kernel with fortification.
[Bug tree-optimization/104009] New: r12-6030-g422f9eb7011b76c1 breaks kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104009
Bug ID: 104009
Summary: r12-6030-g422f9eb7011b76c1 breaks kernel build
Product: gcc
Version: 12.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: tree-optimization
Assignee: unassigned at gcc dot gnu.org
Reporter: siddhesh at gcc dot gnu.org
Target Milestone: ---
Reproducer gleaned from the kernel:
const char *
nlmdbg_cookie2a(unsigned n, char **data)
{
static char buf[255];
unsigned int i, len = sizeof(buf);
char *p = buf;
len--; /* allow for trailing \0 */
for (i = 0 ; i < n ; i++)
{
if (len < 2)
{
__builtin___strcpy_chk(p-3, "...", __builtin_object_size (p-3, 1));
break;
}
p += 2;
len -= 2;
}
*p = '\0';
return buf;
}
$ cat repr.c.031t.early_objsz
;; Function nlmdbg_cookie2a (nlmdbg_cookie2a, funcdef_no=0, decl_uid=1980,
cgraph_uid=1, symbol_order=0)
Computing maximum subobject size for _1:
Visiting use-def links for _1
Visiting use-def links for p_6
Visiting use-def links for p_9
Visiting use-def links for p_14
Found a dependency loop at p_6
Need to reexamine p_14
Need to reexamine p_6
Need to reexamine _1
Visiting use-def links for _1
Need to reexamine _1
Reexamining _1
Visiting use-def links for p_6
Need to reexamine p_6
Reexamining p_6
Visiting use-def links for p_14
Need to reexamine p_14
Reexamining p_14
_1: maximum subobject size 0
p_6: maximum subobject size 255
p_9: maximum subobject size 255
p_14: maximum subobject size 253
const char * nlmdbg_cookie2a (unsigned int n, char * * data)
{
char * p;
unsigned int len;
unsigned int i;
static char buf[255];
char * _1;
long unsigned int _2;
char * _3;
const char * _19;
long unsigned int _20;
:
len_8 = 255;
p_9 =
len_10 = len_8 + 4294967295;
i_11 = 0;
goto ; [INV]
:
if (len_5 <= 1)
goto ; [INV]
else
goto ; [INV]
:
_1 = p_6 + 18446744073709551613;
_20 = __builtin_object_size (_1, 1);
_2 = MIN_EXPR <_20, 0>;
_3 = p_6 + 18446744073709551613;
__builtin___memcpy_chk (_3, "...", 4, _2);
goto ; [INV]
:
p_14 = p_6 + 2;
len_15 = len_5 + 4294967294;
i_16 = i_4 + 1;
:
# i_4 = PHI
# len_5 = PHI
# p_6 = PHI
if (i_4 < n_12(D))
goto ; [INV]
else
goto ; [INV]
:
*p_6 = 0;
_19 =
return _19;
}
Basically since p_6 is an estimate (i.e. the wholesize) and not a precise
value, negative offsets don't quite work. I need to figure out a way to punt
on negative offsets if we're using size estimates instead of precise sizes.
This means that it'll work for dynamic object sizes (because at the moment
they're always precise expressions) but not always for static sizes.
Right now it breaks for dynamic object sizes too, but that's only because
early_objsz treats __builtin_dynamic_object_size as __builtin_object_size to
get an upper bound and ends up zeroing it. So punting should fix that too.
[Bug ipa/101941] [12 Regression] Linux kernel build failure due to retaining fnsplit fragment with __attribute__((__error__))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101941 --- Comment #29 from Siddhesh Poyarekar --- (In reply to Andrew Pinski from comment #28) > (In reply to Martin Liška from comment #26) > > that started with r12-6030-g422f9eb7011b76c1. > > Please file that bug separately and it might be related to PR 103961 which > was just fixed too. It's kinda like PR 103961, but not the same. I'll file a new bug; I've got a reduced reproducer too.
[Bug middle-end/77608] missing protection on trivially detectable runtime buffer overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77608 --- Comment #8 from Siddhesh Poyarekar --- The test case for pr 103961 exposed a flaw in my patch, where assuming wholesize isn't always safe or at least would need more careful consideration. I need to think this through some more.
[Bug tree-optimization/103961] [12 Regression] gcc-12 apparently miscompiles libcap's cap_to_text() function since r12-6030-g422f9eb7011b76c1
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103961 --- Comment #16 from Siddhesh Poyarekar --- Should be fixed with that patch. May I close this or wait for confirmation from the reporter?
[Bug tree-optimization/103961] [12 Regression] gcc-12 apparently miscompiles libcap's cap_to_text() function since r12-6030-g422f9eb7011b76c1
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103961 Siddhesh Poyarekar changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |siddhesh at gcc dot gnu.org Status|NEW |ASSIGNED --- Comment #14 from Siddhesh Poyarekar --- Looking into it.
[Bug middle-end/77608] missing protection on trivially detectable runtime buffer overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77608 Siddhesh Poyarekar changed: What|Removed |Added Status|NEW |ASSIGNED --- Comment #7 from Siddhesh Poyarekar --- I've posted a patch: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587698.html which returns the whole size of an object (that's a thing now, since __bos started handling negative offsets) if the offset is not a constant. It goes on top of the dynamic object sizes patchset. Volatile offsets will need more rework (basically delay the side effects check into tree-object-size), so I'll do that after all of these patches are through.
