Page MenuHomePhabricator

[clang] diagnose_as attribute for Fortify diagnosing like builtins.
AcceptedPublic

Authored by mbenfield on Oct 18 2021, 11:07 AM.

Diff Detail

Event Timeline

mbenfield created this revision.Oct 18 2021, 11:07 AM
mbenfield requested review of this revision.Oct 18 2021, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 11:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pirama added a subscriber: pirama.Oct 18 2021, 11:16 AM

Thanks for this! The idea LGTM, and sounds like a solid way for us to do better about diagnosing FORTIFY'ed calls in Clang. I have a handful of mostly nits/questions for you :)

clang/include/clang/Basic/Attr.td
3869

purely subjective nit: diagnose_as feels a bit too generic. if you agree, does diagnose_as_builtin sound potentially better?

clang/lib/Sema/SemaChecking.cpp
656

nit: const

658

nit: const auto *

660–665

It seems that this is serving as implicit validation of this attribute's members (e.g., if we can't figure out the Function that this DAIAttr is pointing to, we exit gracefully, etc.)

Would it be better to instead do this validation in handleDiagnoseAsAttr, and store the results (which we can then presume are valid here) in the DiagnoseAsAttr?

In that case, it might be easiest to simply store the FunctionDecl that's being referenced, rather than a DRE to it.

clang/lib/Sema/SemaDeclAttr.cpp
1013

nit: const?

clang/test/Sema/warn-fortify-source.c
184 ↗(On Diff #380482)

we generally try to extensively test error cases around new attributes. i'd recommend the following cases:

  • diagnose_as(__function_that_doesnt_exist)
  • diagnose_as(function_i_predeclared_but_which_isnt_a_builtin)
  • diagnose_as(__builtin_memcpy, 1, 2, 3, 4) (too many args)
  • diagnose_as(__builtin_memcpy, 1, 2) (too few args)
  • diagnose_as(__builtin_memcpy, "abc", 2, 3)
  • void test_memcpy(double, double, double) __attribute__((diagnose_as(__builtin_memcpy, 1, 2, 3)); (mismatched param types)
  • diagnose_as(__some_overloaded_function_name)

there're probably more that might be nice, but those seem like a good foundation :)

aaron.ballman added inline comments.Oct 21 2021, 6:27 AM
clang/include/clang/Basic/Attr.td
3869

Agreed on it being a bit generic -- it sounds like this is only useful for Fortify, so I wonder if I'm wrong about that or whether we should name it fortify_diagnose_as_builtin or fortify_diagnostic, etc.

clang/include/clang/Basic/AttrDocs.td
5992–5993

I think we should go into more detail about what the correct order of arguments actually means. The example doesn't make it particularly clear what's going on.

clang/lib/Sema/SemaChecking.cpp
660–663
716–721
731–736
clang/lib/Sema/SemaDeclAttr.cpp
1018–1019

This diagnostic looks incorrect to me -- the call to checkUInt32Argument() diagnoses if the expression is not an appropriately-sized integer. This diagnostic is about the value received being out of bounds.

1026–1030

This also seems a bit wrong -- we don't expect a constant expression, we expect a builtin function ID. You may need to use a new diagnostic here instead of reusing an existing one. Also, we should check that we got an actual builtin function as the expression, shouldn't we?

clang/test/Sema/warn-fortify-source.c
184 ↗(On Diff #380482)

Additional tests:

diagnose_as(some_templated_function, 1, 2, 3)
diagnose_as(some_templated_function_instantiation<int>, 1, 2, 3)
diagnose_as("123", 1, 2, 3)

Also interesting to test is redeclaration behavior:

void my_awesome_func(const char *);

void use1() {
  my_awesome_func(0);
}

[[clang::diagnose_as(__builtin_strlen, 1)]] void my_awesome_func(const char *str) {}

void use2() {
  my_awesome_func(0);
}
clang/include/clang/Basic/Attr.td
3869

(Jumping in a bit late) I second the diagnose_as_builtin name. But then we should check that the attribute is only set on inline builtin declaration (as in Decl::isInlineBuiltinDeclaration) and state so in the documentation.

mbenfield updated this revision to Diff 384451.Nov 3 2021, 8:17 AM

Renamed to diagnose_as_builtin (since two people suggested that name). Let me
know if a name mentioning fortify is preferred.

Validation in handleDiagnoseAsBuiltinAttr:

  • first argument is a builtin function
  • number of arguments matches number of parameters of that function
  • subsequent arguments are integers
  • indices are in bounds for the parameters of the declared function
  • types match for declared function and builtin function

Added diagnostics for the above errors.

Added some consts.

Moved tests to new file attr-diagnose-as-builtin.c.

Added many test cases.

Fixed diagnostic around checkUInt32Argument.

Clarified documentation.

mbenfield marked 8 inline comments as done.Nov 4 2021, 10:06 AM
mbenfield added inline comments.
clang/include/clang/Basic/Attr.td
3869

I think Decl::isInlineBuiltinDeclaration may be the wrong thing to check; it fails for some builtin functions. Instead I'm checking against getBuiltinID. Let me know if you feel this is incorrect.

mbenfield marked 2 inline comments as done.Nov 8 2021, 11:55 AM

Would @george.burgess.iv and/or @aaron.ballman mind taking another look at the latest revision?

LGTM % 2 nits. Please feel free to commit after it LGT @aaron.ballman too. :)

Thanks again!

clang/include/clang/Basic/DiagnosticSemaKinds.td
2961

tiny nit: for consistency with the other options here

clang/test/Sema/attr-diagnose-as-builtin.c
63

nit: can we also add a non-templated overload check in here?

if the diag isn't beautiful, that's fine IMO. just having a test-case to show the expected behavior would be nice

This revision is now accepted and ready to land.Fri, Nov 12, 10:17 AM
mbenfield added inline comments.Fri, Nov 12, 10:26 AM
clang/test/Sema/attr-diagnose-as-builtin.c
63

Sorry, not sure what is being requested. By a "non-templated overload check" do you mean something different from memcpy2 above?

clang/test/Sema/attr-diagnose-as-builtin.c
63

something like the new edit suggestion below is what i have in mind :)

72
mbenfield updated this revision to Diff 387389.Mon, Nov 15, 1:41 PM

s/requires/takes in diagnostic message.

Test case with a non-template overloaded function.

aaron.ballman added inline comments.Thu, Nov 18, 5:19 AM
clang/include/clang/Basic/AttrDocs.td
6011

One thing that's not quite clear from the docs is how to handle variadic arg builtins like __builtin_printf -- how do you specify "the variadic args go here"?

Another thing that's unclear is whether you can apply this attribute to a member function, and if so, how does that change the indexes given for the implicit this argument?

We should add test coverage for both of these situations.

clang/lib/Sema/SemaChecking.cpp
689

We typically don't use top-level const on value types for local variables/parameters (only on pointers or references). (same comment applies elsewhere)

699–701

Please spell out the type.

clang/lib/Sema/SemaDeclAttr.cpp
1006

I don't believe D can ever be null.

1014–1018

Removes curly braces and an else after a return.

1040

Passing in a NamedDecl will be automatically quoted by the diagnostics engine, while passing in a StringRef will not. We prefer syntax to be quoted in diagnostics to reduce confusion.

1055–1057
1061
1065–1066

Please spell out the type as it's not spelled out in the initialization.

1068–1069

This may be fine, but it may also be overly restrictive. Consider:

void f(void *dest, char val, size_t n) __attribute__((diagnose_as_builtin(__builtin_memset, 1, 2, 3))) {}

I think this will get diagnosed because of the type mismatch between char and int, won't it? But notionally, the argument passed to val will be promoted to int anyway, so they're morally equivalent. This would be a good test case to add.

FWIW, I prefer erring on the side of being overly restrictive (we can relax restrictions more easily than we can add new ones), which is why I think this is fine for now.

mbenfield updated this revision to Diff 389355.Tue, Nov 23, 4:45 PM

Error on member function. Test case for this.

Test case for attribute applied to a non-function declaration.

Handle variadic functions. Document this. Test case for variadic functions.

Remove const from some local values.

Spell out some types.

Change superflous dyn_cast_or_null to dyn_cast.

Remove some curly braces.

Pass in a NamedDecl directly rather than its name to a diagnostic.

Add a test case for types not being identical.

aaron.ballman added inline comments.Tue, Nov 30, 7:13 AM
clang/include/clang/Basic/AttrDocs.td
5994

We should mention that the attribute cannot be applied to a member function (and if you decide it shouldn't apply to a static member function either, I'd call that out explicitly as well).

6013
6025
clang/lib/AST/Decl.cpp
2021 ↗(On Diff #389355)

Spurious whitespace change can be reverted.

clang/lib/Sema/SemaDeclAttr.cpp
1006–1009

Sorry, I should have suggested this earlier -- might as well assert if D is something we should have already validated is a FunctionDecl before calling this helper.

1011–1014

Hmm, this is getting into real nit territory, but, shouldn't it be fine to use a static member function?

1021–1022

Sorry, missed this one earlier as well (no else after a return per our usual coding guidelines).

1059–1061
1065
clang/test/Sema/attr-diagnose-as-builtin.c
105

It'd be good to add another test case for static member functions just to show whether we do or do not support them.

mbenfield updated this revision to Diff 391067.Wed, Dec 1, 9:54 AM

Revert spurious whitespace change.

assert(D)

No else after return.

Allow attribute to be applied to static member functions and test this.

Document that it can't be applied to a non-static member function.

mbenfield updated this revision to Diff 391076.Wed, Dec 1, 10:40 AM
mbenfield marked 8 inline comments as done.

Replace DeclFD->getName() with just DeclFD.

mbenfield marked an inline comment as done.Wed, Dec 1, 10:43 AM
aaron.ballman added inline comments.Tue, Dec 7, 10:16 AM
clang/include/clang/Basic/Attr.td
3873

Why does this need to be late parsed? I don't think the arguments to the attribute are ones that name class members or anything like that.

clang/lib/Sema/SemaDeclAttr.cpp
1006–1009

Sorry, my suggestion wasn't as clear as it could have been -- cast<> does the assertion for you, so you can switch dyn_cast to cast and remove the explicit assert here.

@aaron.ballman Look to you like this is ready to land?