Page MenuHomePhabricator

[clang] Implement -fstrict-flex-arrays=3
ClosedPublic

Authored by void on Sep 29 2022, 1:43 PM.

Details

Summary

The -fstrict-flex-arrays=3 is the most restrictive type of flex arrays.
No number, including 0, is allowed in the FAM. In the cases where a "0"
is used, the resulting size is the same as if a zero-sized object were
substituted.

This is needed for proper _FORTIFY_SOURCE coverage in the Linux kernel,
among other reasons. So while the only reason for specifying a
zero-length array at the end of a structure is for specify a FAM,
treating it as such will cause _FORTIFY_SOURCE not to work correctly;
__builtin_object_size will report -1 instead of 0 for a destination buffer
size to keep any kernel internals from using the deprecated members as
fake FAMs.

For example:

struct broken {
    int foo;
    int fake_fam[0];
    struct something oops;
};

There have been bugs where the above struct was created because "oops" was
added after "fake_fam" by someone not realizing. Under __FORTIFY_SOURCE, doing:

memcpy(p->fake_fam, src, len);

raises no warnings when __builtin_object_size(p->fake_fam, 1) returns -1 and
will stomp on "oops."

Omitting a warning when using the (invalid) zero-length array is how GCC
is treating -fstrict-flex-arrays=3. A warning in that situation is likely
an irritant, because requesting this option level is explicitly requesting
this behavior.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836

Diff Detail

Event Timeline

void created this revision.Sep 29 2022, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 1:43 PM
void requested review of this revision.Sep 29 2022, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 1:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kees added a comment.Sep 29 2022, 4:53 PM

Excellent! Thank you for getting this prepared. Having this properly managed in the kernel means we don't have to do horrible ugly hacks to work around old 0-length arrays in UAPI (which have all been unioned with a proper flexible array now).

kees added inline comments.Sep 29 2022, 11:28 PM
clang/test/CodeGen/bounds-checking-fam.c
35–53

I would expect to see here:

struct Zero {
  int a[0];
};

// CHECK-LABEL: define {{.*}} @{{.*}}test_zero{{.*}}(
int test_one(struct One *p, int i) {
  // CHECK-STRICT-0-NOT: @__ubsan
  // CHECK-STRICT-1-NOT: @__ubsan
  // CHECK-STRICT-2-NOT: @__ubsan
  // CHECK-STRICT-3:     call void @__ubsan_handle_out_of_bounds_abort(
  return p->a[i] + (p->a)[i];
}

I think this is missing a change in SemaChecking.cpp too. It would probably be better to first merge https://reviews.llvm.org/D134791 so that you don't have to patch (too much) multiple places :-)

clang/test/CodeGen/bounds-checking-fam.c
7–9

Why adding -02 here?

clang/test/CodeGen/object-size-flex-array.c
45

This one worries me a bit, as an array of size 0 is invalid C, unless you consider the extension of it being a FAM. Shouldn't we emit a warning or something here? @kees what meaning would you give to that construct under -fstrict-flex-arrays=3 ?

clang/test/SemaCXX/array-bounds-strict-flex-arrays.cpp
14

update the comment here and below?

kees added inline comments.Oct 5 2022, 11:05 PM
clang/test/CodeGen/object-size-flex-array.c
45
type identifier[0]

when not a fake FAM is the same as:

struct { } identifier

It's addressable with no size.

FWIW, this is how GCC is treating it, and opted for no warning. The warning gains nothing and is likely an irritant: if someone is requesting =3, they want this behavior. If they didn't, they'd just use -Wzero-length-array. In particular, the Linux kernel is in the position of needing to have zero-length arrays (legacy userspace API) alongside real FAMs, even if we don't reference the zero-length members. So we cannot use '-Wzero-length-array', but we want to make sure no zero-length arrays will ever be used as fake FAMs, as a code quality/style enforcement. This is especially true of FORTIFY_SOURCE, where __bos() needs to report 0 instead of -1 for such a destination buffer size, so that we immediately trip compile-time (or at worst, run-time) warnings, to keep any kernel internals from using the deprecated members as a fake FAM.

Take this case:

struct broken {
    int foo;
    int fake_fam[0];
    struct something oops;
};

There have been bugs where the above struct was created because "oops" got added after "fake_fam" by someone not realizing. Under FORTIFY_SOURCE, doing:

memcpy(p->fake_fam, src, len);

raises no warning when __bos(p->fake_fam, 1) returns -1 and will happily stomp on "oops". If __bos() returns 0, we can compile-time (or run-time) block the memcpy. (And this holds for -fsanitize=bounds as well: if it is considered to be unknown size, it won't trip on access, but if it's 0-sized, it'll trip.)

So, we can't keep zero-length arrays out of the kernel, but we want to be able to enforce that if they DO show up, they will trip warnings quickly.

clang/test/CodeGen/object-size-flex-array.c
45

Thanks for clarifying the situation, esp. the kernel background. This looks like a quite specific scenario, but it's fine with me.

void edited the summary of this revision. (Show Details)Oct 7 2022, 2:10 AM
void marked an inline comment as done.
void added inline comments.
clang/test/CodeGen/bounds-checking-fam.c
7–9

Mostly because that's how Linux compiles its files. It also mirrors the test below. I'm not super married to it if you'd rather I omit it.

clang/test/CodeGen/object-size-flex-array.c
45

I'll add @kees's comment to the commit message.

void updated this revision to Diff 466011.Oct 7 2022, 2:15 AM

Update comments to reflect reality.

void added a reviewer: rsmith.Oct 10 2022, 4:35 PM
void added a subscriber: rsmith.

@rsmith, @serge-sans-paille, and @kees, I need some advice. There's a test in clang/test/CodeGen/bounds-checking.c that's checking bounds stuff on unions. The behavior is...weird to me. It says that an array of 0 or 1 is a FAM, but one larger is not (see below). That seems counter to how structs are handled. If this is true, then the check in clang/lib/AST/Expr.cpp also needs to be updated...

union U { int a[0]; int b[1]; int c[2]; };                                             
  
// CHECK-LABEL: define {{.*}} @f4                                                      
int f4(union U *u, int i) {
  // a and b are treated as flexible array members.
  // CHECK-NOT: @llvm.ubsantrap                                                        
  return u->a[i] + u->b[i] + u->c[i];                                                  
  // CHECK: }
}                                                                                      
  
// CHECK-LABEL: define {{.*}} @f5
int f5(union U *u, int i) {
  // c is not a flexible array member.
  // NONLOCAL: call {{.*}} @llvm.ubsantrap                                             
  return u->c[i];
  // CHECK: }                                                                          
}
serge-sans-paille added a comment.EditedOct 10 2022, 10:35 PM

@rsmith, @serge-sans-paille, and @kees, I need some advice. There's a test in clang/test/CodeGen/bounds-checking.c that's checking bounds stuff on unions. The behavior is...weird to me. It says that an array of 0 or 1 is a FAM, but one larger is not (see below). That seems counter to how structs are handled. If this is true, then the check in clang/lib/AST/Expr.cpp also needs to be updated...

I second the opinion here. C99 says nothing about flexible array member for unions, that's already a "language extension". (and so not be considered as FAM by -fstrict-flex-arrays=3)
Both GCC and Clang implement that extension for array of size 0 and 1, see https://godbolt.org/z/1xYMYq75s. That's the *legacy* behavior of Clang.

We may want to harmonize with struct behavior (for consistency etc) but I'd advocate to so in a separate patch.

kees added a comment.Oct 10 2022, 10:52 PM

@rsmith, @serge-sans-paille, and @kees, I need some advice. There's a test in clang/test/CodeGen/bounds-checking.c that's checking bounds stuff on unions. The behavior is...weird to me. It says that an array of 0 or 1 is a FAM, but one larger is not (see below).

Note that union vs struct shouldt't matter. A union is just a struct where all members are "trailing". ;)

f5 looks like a broken test that didn't realize that N-sized trailing arrays are considered fake FAMs. This would explain some of the unexpected behavior I've seen with -fsanitize=bounds under Clang vs GCC:
https://godbolt.org/z/5v3evhMqq
Here GCC (correctly) accepts all as fake FAMs.

Note that GCC has an option -fsanitize=strict-bounds that changes the behavior to treating [N] and [1] as fixed size, but _not_ [0]. The plan is for GCC to make this an alias of -fstrict-flex-arrays=2, and then have UBSAN Bounds correctly tied to the -fstrict-flex-arrays level.

That seems counter to how structs are handled. If this is true, then the check in clang/lib/AST/Expr.cpp also needs to be updated...

I would expect diagnostics, __builtin_object_size(), __builtin_dynamic_object_size(), and -fsanitize=bounds to all agree on the definition of fake FAMs, which is all controlled by -fstrict-flex-arrays level.

kees added a comment.Oct 10 2022, 11:06 PM

I second the opinion here. C99 says nothing about flexible array member for unions, that's already a "language extension". (and so not be considered as FAM by -fstrict-flex-arrays=3)

To be super pedantic, C99 implies a FAM in a union is illegal. 6.7.2.1.16 says "As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a flexible array member." The implication is that such a state ("more than one named member") isn't possible in a union.

But in real-world usage, this definition isn't useful and flies in the face of actual (fake) FAM usage. Having fake FAMs in unions is _very_ common in the Linux kernel, and they even appear alone in structs. There is no pragmatic reason for the C99 limitation, and it's needlessly enforced only for "real" FAMs. But this is a separate issue we can solve separately.

Both GCC and Clang implement that extension for array of size 0 and 1, see https://godbolt.org/z/1xYMYq75s. That's the *legacy* behavior of Clang.

We may want to harmonize with struct behavior (for consistency etc) but I'd advocate to so in a separate patch.

I just want to repeat for clarity: this isn't about union vs struct. This is about UBSAN vs not. Here is the same behavior, shown with a struct:
https://godbolt.org/z/4TbWYP4f9
Clang's -fsanitize=array-bounds is misbehaving.

dblaikie added inline comments.
clang/test/CodeGen/bounds-checking-fam.c
7–9

Yeah, generally Clang tests don't run LLVM optimizations - clang should be checking the output of Clang's IRGen, not the optimizations that LLVM performs.

Not sure how many Clang codegen tests disable llvm optimizations entirely, but that's basically/should be the default (-disable-llvm-optzns, I think that's the flag anyway).

If LLVM's output is so verbose it's hard to check reliably/non-brittally then maybe running some optimizations to simplify things might be acceptable.

void updated this revision to Diff 469805.Oct 21 2022, 3:41 PM

Integrate the new flag level into the FAM predicate.

void updated this revision to Diff 469807.Oct 21 2022, 3:46 PM

Add a release note.

void marked 2 inline comments as done.Oct 21 2022, 3:47 PM
void added a comment.Oct 21 2022, 3:52 PM

This should be ready for review. PTAL.

clang/include/clang/Basic/LangOptions.h
60

Looks unrelated to this patch :-)

231–232

Looks unrelated to this patch.

clang/lib/AST/ExprConstant.cpp
14099

fascinatingly off topic. But gives a good hint on what you're working on ;-)

void added inline comments.Oct 24 2022, 11:13 AM
clang/include/clang/Basic/LangOptions.h
60

I don't see a change here.

231–232

Or here.

clang/lib/AST/ExprConstant.cpp
14099

I'm not sure what you mean.

I second the opinion here. C99 says nothing about flexible array member for unions, that's already a "language extension". (and so not be considered as FAM by -fstrict-flex-arrays=3)

To be super pedantic, C99 implies a FAM in a union is illegal. 6.7.2.1.16 says "As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a flexible array member." The implication is that such a state ("more than one named member") isn't possible in a union.

The C standard says "structure" when it means struct and "union" when it means union, and "structure and/or union" when it means both/either, so this constraint is not implied -- it's explicitly stated. A FAM in a union is a constraint violation per 6.7.2.1. (Note: C compilers are not consistent about getting this constraint correct. Clang and GCC both get it right, ICC, MSVC, SDCC all get it wrong.)

But in real-world usage, this definition isn't useful and flies in the face of actual (fake) FAM usage. Having fake FAMs in unions is _very_ common in the Linux kernel, and they even appear alone in structs. There is no pragmatic reason for the C99 limitation, and it's needlessly enforced only for "real" FAMs. But this is a separate issue we can solve separately.

A *fake* FAM is undefined behavior and thus fair game for implementations to define the behavior of in whatever way we'd like, so long as developers know it's not portable and it may explode on other compilers. A real FAM requires at least a diagnostic, but we're free to make it a pedantic warning diagnostic if we want. If we feel strongly about the limitation being needless, we could propose to standardize support for it if implementations that accept a real FAM in a union basically all agree on the behavior. (Agreed that this is a separate issue from this patch though.)

One thing I would appreciate is, once we're getting close to done with the work around flexible arrays, is to explicitly document our implementation-defined behaviors around flexible arrays in https://clang.llvm.org/docs/LanguageExtensions.html. Right now, what is and isn't a flexible array to Clang is really complex and not having the rules written down anywhere is not helpful to users. These cleanup efforts are fantastic and seem like a good opportunity for us to rectify the lack of docs.

clang/docs/ReleaseNotes.rst
399

I think another way to phrase this is that level 3 only supports flexible array members as the feature is defined by the C standard, aka, this is standards-conforming mode. WDYT?

void updated this revision to Diff 470561.Oct 25 2022, 10:49 AM

Update the release notes to something resembling English. Also add a better
explanation of what "3" means.

void marked an inline comment as done.Oct 25 2022, 10:49 AM
void added a comment.Oct 25 2022, 11:00 AM

One thing I would appreciate is, once we're getting close to done with the work around flexible arrays, is to explicitly document our implementation-defined behaviors around flexible arrays in https://clang.llvm.org/docs/LanguageExtensions.html. Right now, what is and isn't a flexible array to Clang is really complex and not having the rules written down anywhere is not helpful to users. These cleanup efforts are fantastic and seem like a good opportunity for us to rectify the lack of docs.

Sure. Do you want that done with this change?

aaron.ballman accepted this revision.Oct 25 2022, 11:34 AM

One thing I would appreciate is, once we're getting close to done with the work around flexible arrays, is to explicitly document our implementation-defined behaviors around flexible arrays in https://clang.llvm.org/docs/LanguageExtensions.html. Right now, what is and isn't a flexible array to Clang is really complex and not having the rules written down anywhere is not helpful to users. These cleanup efforts are fantastic and seem like a good opportunity for us to rectify the lack of docs.

Sure. Do you want that done with this change?

Up to you -- I'd be fine if you preferred to do it in a follow-up given that it's somewhat indirectly related to adding support for 3. Changes LGTM currently, but please wait a day or so before landing in case @serge-sans-paille or @kees have comments.

This revision is now accepted and ready to land.Oct 25 2022, 11:34 AM
void added a comment.Oct 26 2022, 4:51 PM

Quick ping to @kees and @serge-sans-paille for any comments. :)

kees accepted this revision.Oct 26 2022, 4:58 PM

The self-tests all look correct to me, so I expect it is working how I'd expect. I haven't had a chance to do kernel builds with this yet, but I'm hoping to make time soon. I'd say if others are happy, let's land it, and if anything unexpected happens we can fix those issues if they appear. :)

This revision was automatically updated to reflect the committed changes.