On 9/4/19 3:15 PM, Martin Sebor wrote:
> While testing some other buffer overflow improvements I was
> reminded that GCC doesn't detect even some basic instances
> of overflowing struct members involving strcpy calls with
> non-constant strings, with or without _FORTIFY_SOURCE. For
> example:
>
> struct S { char a[4]; void (*pf)(void); };
>
> extern struct S a[];
>
> void f (void)
> {
> char s[] = "123456789";
> strcpy (a[0].a, s);
> }
>
> This is because the strlen pass that computes the length of
> non-constant strings like s above isn't yet integrated with
> the -Wstringop-overflow/_FORTIFY_SOURCE checking.
>
> But the strlen pass does call into the wrestrict pass to check
> for overlapping copies (mainly to avoid issuing meaningless
> -Wrestrict warnings or mentioning invalid offsets in then), and
> the wrestrict pass checks for invalid offsets and ordinarily
> diagnoses those.
>
> However, the invalid offset detection isn't quite robust enough
> to detect these cases (it gives up too early), and so the first
> line of defense fails.
>
> The attached patch improves the wrestrict pass to also detect
> these invalid offsets. The improvement relies on the recently
> introduced component_size() function. Testing the change in
> turn exposed a bug in the function (relying on the size of
> an array to determine if it was the member[1] kind rather than
> considering its domain. So the patch also corrects that bug
> With the patch applied the out-of-bounds access above is
> diagnosed:
>
> warning: ‘strcpy’ offset [4, 9] from the object at ‘a’ is out of the
> bounds of referenced subobject ‘a’ with type ‘char[4]’ at offset 0
> [-Warray-bounds]
>
> -Warray-bounds isn't the ideal warning to issue and ultimately,
> the strcpy member overflow detection will need to be done under
> -Wstringop-overflow, and trigger aborts at runtime with
> _FORTIFY_SOURCE. But that's a project of its own (possibly
> involving moving the -Wstringop-overflow checking code from
> builtins.c into the strlen pass itself).
>
> Tested on x86_64-linux.
>
> Martin
>
> PS I can think of little in terms of "infrastructure" in this
> work that could be split out into a separate patch, except
> perhaps the component_size change. But given how small
> the function is it doesn't seem necessary.
The only dense changes were the gimple-ssa-warn-restrict stuff, the rest
was pretty easy to see what you were doing. So I don't think it's
necessary.
>
> gcc-91631.diff
>
> PR middle-end/91631 - buffer overflow into an array member of a declared
> object not detected
>
> gcc/ChangeLog:
>
> PR middle-end/91631
> * builtins.c (component_size): Correct trailing array computation,
> rename to component_ref_size and move...
> (compute_objsize): Adjust.
> * gimple-ssa-warn-restrict.c (builtin_memref::refsize): New member.
> (builtin_access::strict): Do not consider mememmove.
> (builtin_access::write_off): New function.
> (builtin_memref::builtin_memref): Initialize refsize.
> (builtin_memref::set_base_and_offset): Adjust refoff and compute
> refsize.
> (builtin_memref::offset_out_of_bounds): Use ooboff input values.
> Handle refsize.
> (builtin_access::builtin_access): Intialize dstoff to destination
> refeence offset here instead of in maybe_diag_overlap. Adjust
> referencess even to unrelated objects. Adjust sizrange of bounded
> string functions to reflect bound. For strcat, adjust destination
> sizrange by that of source.
> (builtin_access::strcat_overlap): Adjust offsets and sizes
> to reflect the increase in destination sizrange above.
> (builtin_access::overlap): Do not set dstoff here but instead
> in builtin_access::builtin_access.
> (check_bounds_or_overlap): Use builtin_access::write_off.
> (maybe_diag_access_bounds): Add argument. Add informational notes.
> (dump_builtin_memref, dump_builtin_access): New functions.
> * tree.c (component_ref_size): ...to here.
> * tree.h (component_ref_size): Declare.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/91631
> * /c-c++-common/Warray-bounds-3.c: Correct expected offsets.
> * /c-c++-common/Warray-bounds-4.c: Same.
> * gcc.dg/Warray-bounds-39.c: Remove xfails.
> * gcc.dg/Warray-bounds-44.c: New test.
> * gcc.dg/Warray-bounds-45.c: New test.
Couple nits in the ChangeLog. mememmove and Intialize.
ChangeLog also fails to mention tree-ssa-strlen.c changes.
OK with the ChangeLog nits fixed. If you intended to include hte
tree-ssa-strlen changes in this patch, then please add them to the
ChangeLog.
jeff
