[PATCH] D135727: [clang] Correct sanitizer behavior in union FAMs

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D135727#3853896 <https://reviews.llvm.org/D135727#3853896>, @void wrote:

> @kees @serge-sans-paille: It appears to me that a terminating array of size > 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first failure above (`clang/test/Sema/array-bounds-ptr-arith.c`) shows that. It turns out that the same failure occurs in that testcase when the array isn't the last in a structure, so I'll change it.

Okay, what the heck is even going on in this test? The diagnostic appears to think the array changes in size based on the cast??

  <source>:16:24: warning: the pointer incremented by 80 refers past the end of the array (that contains 64 elements) [-Warray-bounds-pointer-arithmetic]
          return (void *)es->s_uuid + 80;
                         ^            ~~

s_uuid is 8, not 64. 64 would be 8 "void *"s.  This seems like a very very broken diagnostic?

These should all trip the diagnostic, but don't:

  es->s_uuid + 8; /* this is past the end */
  (void *)es->s_uuid + 9; /* this is past the end by 1, but doesn't trip because it thinks it's suddenly 8 times larger */

For -Warray-bounds, GCC isn't fooled by the "void *" casts (but doesn't have -Warray-bounds-arithmetic):
https://godbolt.org/z/5eqEEv4of

Note that while the diagnostics of both GCC and Clang complain only about >1 terminal arrays, they both return -1 for `__builtin_object_size` regardless of length.

So, we're facing, again, a disconnect between diagnostics, bos, and sanitizer. GCC's sanitizer follows bos rules, where-as Clang's sanitizer followed diagnostics rules. Given that bos is used for run-time analysis, it follows that sanitizer and bos should match.

> There's another failure that I'm not too sure about. The `Clang.SemaCXX::array-bounds.cpp` failure is due to a union that's acting like an FAM. I have a question for you. Should `a` and `c` in the union in this code be considered an FAM?

This test looks correct to me:

>   struct {
>     union {
>       short a[2]; // expected-note 4 {{declared here}}
>       char c[4];
>     };
>     int ignored;
>   };

I would **not** expect this to warn or trap because it's not the trailing member of the //structure//.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135727/new/

https://reviews.llvm.org/D135727