Page MenuHomePhabricator

Add an attribute that causes clang to emit fortified calls to C stdlib functions
ClosedPublic

Authored by erik.pilkington on Feb 7 2019, 11:36 AM.

Details

Summary

This attribute applies to declarations of C stdlib functions (sprintf, memcpy...) that have known fortified variants (sprintf_chk, memcpy_chk, ...). When applied, clang will emit calls to the fortified variant functions. We need this attribute because its impossible to write gnu_inline-style wrappers to variadic functions because we don't support __builtin_va_arg_pack, and don't intend to (see https://reviews.llvm.org/D57635).

Fixes rdar://47905754

Thanks for taking a look!
Erik

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 11:36 AM
erik.pilkington edited the summary of this revision. (Show Details)Feb 7 2019, 6:50 PM
rjmccall added inline comments.Feb 7 2019, 7:23 PM
clang/include/clang/Basic/AttrDocs.td
986 ↗(On Diff #185836)

Probably worth clarifying somewhere in here that only a specific set of stdlib functions are supported. I don't know that it's important to spell that set out.

clang/lib/CodeGen/CGBuiltin.cpp
1490 ↗(On Diff #185836)

Could you move the body of this into a helper function? This function is really far too large as it is.

1516 ↗(On Diff #185836)

I think this might be over-commented. :)

Could you just put smaller comments on groups of cases, like:

// All of these take the destination object size as the final argument.

or

// These take flags and the destination object size immediately before the format string.
clang/test/Sema/fortify-std-lib.c
6 ↗(On Diff #185836)

Silly comment, but: you typo'd "anything" here.

erik.pilkington marked 5 inline comments as done.

Address John's comments.

clang/include/clang/Basic/AttrDocs.td
986 ↗(On Diff #185836)

I just copied a list in, there really aren't that many, so may as well just be explicit.

rjmccall accepted this revision.Feb 8 2019, 11:54 AM

LGTM, thanks!

clang/include/clang/Basic/AttrDocs.td
986 ↗(On Diff #185836)

WFM

This revision is now accepted and ready to land.Feb 8 2019, 11:54 AM
aaron.ballman added inline comments.Feb 8 2019, 11:59 AM
clang/include/clang/Basic/Attr.td
1572 ↗(On Diff #186013)

Nothing in the docs describes these two arguments.

clang/include/clang/Basic/AttrDocs.td
987 ↗(On Diff #186013)

And we don't have Annex K versions for any of these, like memset_s()?

erik.pilkington marked 2 inline comments as done.

Describe what the arguments do in the docs.

clang/include/clang/Basic/AttrDocs.td
987 ↗(On Diff #186013)

No, it doesn't look like clang even recognizes these yet, and our c library doesn't define _chk functions for them. Whenever that happens, it would good to support them though I guess.

aaron.ballman added inline comments.Feb 11 2019, 5:46 AM
clang/include/clang/Basic/AttrDocs.td
987–989 ↗(On Diff #186028)

Maybe I'm being dense, but I still have no idea what I'd pass for either of these arguments. When I hear "type", I immediately assume I should pass in something like 'int', but that seems wrong given that this is an integer argument. Is there some public documentation we could link to with a "for more information, see <blah>" snippet? Similar for the fortified format function flag.

However, I'm also starting to wonder why this attribute is interesting to users. Why not infer this attribute automatically, since there's only a specified set of standard library functions that it can be applied to anyway? Is it a bad idea to try to infer this for some reason?

erik.pilkington marked an inline comment as done.Feb 11 2019, 11:24 AM
erik.pilkington added inline comments.
clang/include/clang/Basic/AttrDocs.td
987–989 ↗(On Diff #186028)

Yeah, I guess we could link to GCC's docs for __builtin_object_size, I'll update this. I think the flag argument has something to do with enabling checking %n output parameters, but I'm not totally sure and don't want to spread any misinformation in the clang docs. On our implementation, the value is just ignored.

This attribute would never really be used by users that aren't C library implementers. The problem with doing this automatically is that library users need to be able to customize the checking mode by defining the _FORTIFY_SOURCE macro to a level in their .c files. We can't do this based on the presence of that macro for a couple reasons, firstly, I'm not sure we can assume that all *_chk variants are present just because _FORTIFY_SOURCE is defined (whether the _chk variants are present seems like a decision best left to the library). And secondly because that would mean that clang -E t.c -o - | clang -xc - would generate different code from clang t.c. Given that, it seems like an attribute is the best customization point.

Link to the GCC docs for __builitn_object_size.

aaron.ballman accepted this revision.Feb 11 2019, 1:42 PM

LGTM aside from some very minor nits.

clang/include/clang/Basic/AttrDocs.td
987–989 ↗(On Diff #186028)

Thank you for the explanation -- I agree that an attribute probably makes sense then. I'd appreciate a note in the docs saying something along the lines of "This attribute is intended for use within standard C library implementations and should not generally be used for user applications." or some such. WDYT?

994–1012 ↗(On Diff #186300)

Rather than enumerate a long list, can you shorten it vertically by grouping the functions? e.g., strpcpy, strcpy, et. al. on the same line

clang/lib/CodeGen/CGBuiltin.cpp
1491 ↗(On Diff #186300)

Is the explicit trailing return here needed? I would have assumed this could be inferred.

1548 ↗(On Diff #186300)

auto *

1574 ↗(On Diff #186300)

const auto * and some other identifier than Attr (so it won't conflict with the type name).

clang/lib/Sema/SemaDeclAttr.cpp
6437 ↗(On Diff #186300)

lol, I'll fix that typo after you land your patch.

erik.pilkington marked 8 inline comments as done.Feb 11 2019, 3:21 PM
erik.pilkington added inline comments.
clang/include/clang/Basic/AttrDocs.td
987–989 ↗(On Diff #186028)

Sure, I added that in the commit.

clang/lib/Sema/SemaDeclAttr.cpp
6437 ↗(On Diff #186300)

Thanks, guess you're less lazy then me :)

Closed by commit rL353765: Add a new attribute, fortify_stdlib (authored by epilk, committed by ). · Explain WhyFeb 11 2019, 3:21 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 3:21 PM
erik.pilkington marked 2 inline comments as done.Mar 13 2019, 2:36 PM
erik.pilkington added a subscriber: jyknight.

I reverted this in r356103:

Revert "Add a new attribute, fortify_stdlib"

This reverts commit r353765. After talking with our c stdlib folks, we decided
to use the existing pass_object_size attribute to implement _FORTIFY_SOURCE
wrappers, like Bionic does (I didn't realize that pass_object_size could be used
for this purpose). Sorry for the flip/flop, and thanks to James Y. Knight for
pointing this out to me.