[clang-tidy] Add bugprone-unsafe-functions checker.

[futogergely]

Summary

Checks for unsafe functions listed in CERT C Coding Standard
Recommendation MSC24-C and Rule MSC33-C.

For the listed functions, an alternative, more secure replacement is
suggested, if available. The checker heavily relies on the functions
from annex K (Bounds-checking interfaces) of C11.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

20

Why this could not be member of check class?

53

Why not to use plain assignment? See readability-simplify-boolean-expr.

60

Early return will be better.

70

Please elide braces.

81

Please elide braces.

85

Could be const auto *, because type is spelled in same statement.

86

Could be const auto *, because type is spelled in same statement.

93

Could be const auto *, because type is spelled in same statement.

94

Could be const auto *, because type is spelled in same statement.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h

13

Please separate include statement and namespaces with empty line.

14

Please separate with empty line.

clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst

6 ↗

(On Diff #303633)

Please make first statement same as in Release Notes. Please enclose all preprocessor variables and function names in double back-ticks.

15 ↗

(On Diff #303633)

Link to original rule should be at the end. See other documentation as example.

17 ↗

(On Diff #303633)

Please remove space before colon and separate code with empty line.

18 ↗

(On Diff #303633)

Should be .. code-block:: c++. See other documentation as formatting example.

clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp

5 ↗

(On Diff #303633)

Please separate with empty line.

[steakhal]

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.
What do you think?

There is a mismatch between your code and docs too, regarding the function you check for.
In the code you match for vsprintf, vsscanf, vswprintf, vswcanf, wcrtomb, wcscat, but none of these are mentioned in the docs.

The checker warns only if STDC_LIB_EXT1 macro is defined and the value of STDC_WANT_LIB_EXT1 macro is 1 in this case it suggests the corresponding functions from Annex K instead the vulnerable function.

I would suggest mentioning these macros and their purpose in the docs.
Eg. that the STDC_WANT_LIB_EXT1 should be defined to 1 but the other is left to the implementation.

That being said, I would request more tests, demonstrating that this macro detection works accordingly.

This checker might be a bit noisy. Have you tried it on open-source projects?
If it is, we should probably note that in the docs as well.

In the tests, It is a good practice to demonstrate that the offered recommendation does not trigger yet another warning.
Don't forget to put a no-warning to highlight that.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

31

Hm, it must be a typo.
vswcanf -> vswscanf

By the same token, I miss these functions from this enumeration.
vfwprintf, vfwscanf, vprintf, vwprintf, vwscanf
However, they are present in the cert table you mentioned.

34

Why is strlen an obsolescent function? I don't feel it justified, even if there was a comment about it on the cert page.

70

Please also check if the value is 1, as your summary and the standard requires.

78–80

88–90

We should probably use llvm::Twine instead of multiple std::string constructions.
I might be wrong though.

90

can -> should
The same at the other diagnostic message.

92

This and the previous if statement is exclusive, am I right?
If we have a match, that is either a callexpr or a vardecl.

I would prefer a return at the end of the previous if and an assert here that the FunctionPointer must be non-null.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h

28

It should be private.

clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst

10 ↗

(On Diff #303633)

Another typo.

19–23 ↗

(On Diff #303633)

I'm not convinced by this example.

AFAIK, the intended usage is to define the __STDC_WANT_LIB_EXT1__ to 1 before the string.h header included.
It's important to provide a good example, as this checker will be silent if not used correctly.

23 ↗

(On Diff #303633)

We should probably align this comment as well.

[ktomi996]

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

What do you think?

There is a mismatch between your code and docs too, regarding the function you check for.
In the code you match for vsprintf, vsscanf, vswprintf, vswcanf, wcrtomb, wcscat, but none of these are mentioned in the docs.

The checker warns only if STDC_LIB_EXT1 macro is defined and the value of STDC_WANT_LIB_EXT1 macro is 1 in this case it suggests the corresponding functions from Annex K instead the vulnerable function.

I would suggest mentioning these macros and their purpose in the docs.
Eg. that the STDC_WANT_LIB_EXT1 should be defined to 1 but the other is left to the implementation.

That being said, I would request more tests, demonstrating that this macro detection works accordingly.

This checker might be a bit noisy. Have you tried it on open-source projects?
If it is, we should probably note that in the docs as well.

In the tests, It is a good practice to demonstrate that the offered recommendation does not trigger yet another warning.
Don't forget to put a no-warning to highlight that.

[steakhal]

In D91000#2465514, @ktomi996 wrote:

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.
IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

[lebedev.ri]

In D91000#2469880, @steakhal wrote:

In D91000#2465514, @ktomi996 wrote:

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

[steakhal]

In D91000#2469884, @lebedev.ri wrote:

In D91000#2469880, @steakhal wrote:

In D91000#2465514, @ktomi996 wrote:

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

I don't see any value in disabling the check for eg. setbuf. Even the man page says that you should use the setvbuf instead.
Why would anyone disable this if one already wants diagnostics for the CERT rule it implements?

IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

[lebedev.ri]

In D91000#2469894, @steakhal wrote:

In D91000#2469884, @lebedev.ri wrote:

In D91000#2469880, @steakhal wrote:

In D91000#2465514, @ktomi996 wrote:

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

I don't see any value in disabling the check for eg. setbuf. Even the man page says that you should use the setvbuf instead.
Why would anyone disable this if one already wants diagnostics for the CERT rule it implements?

IMO if the CERT rule says that don't use any of these functions but use these other functions instead.
If we don't check all of them in the list, this checker is incomplete. The name of this checker might lead the user to a false sense of security.

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?
Or for them to be actually used?

[steakhal]

In D91000#2469898, @lebedev.ri wrote:

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?

Initially, yes. I think one could learn a lot from contributing to any project.
It's not inherently a bad thing to combine these two.

Or for them to be actually used?

I want a useful checker, that's why I highlighted some of my concerns and suggested a way forward.
It might not be useful to everyone as it tries to implement a domain-specific CERT rule, but it's still up to the user to enable this.
I think we should ask, who is the audience of this checker?
I assume only the users who are interested in the CERT guideline would use this. At this point, we should be clear about what we are checking for.
I think it's OK, to say that only a part of the rule is implemented, and we should carefully document this fact. But IMO one should go the extra mile to try hard and implement the other parts of the rule as well.
Like, matching on the setbuf is not that hard really. The rest of the missing functions probably fall into the same category.

I might be wrong on this though.
Keep in mind that I'm not tidy dev, so take my opinion with a pinch of salt.

[aaron.ballman]

In D91000#2469898, @lebedev.ri wrote:

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?
Or for them to be actually used?

We have an existing CERT module for CERT-specific checks. When the check is honoring a published coding standard, that's usually sufficient justification -- but I do think the check should implement what the CERT recommendation says. If the CERT recommendation seems deficient in some way, it's reasonable to reach out to the authors (they allow comments on their wiki and are pretty good about responding to those comments) for clarification or to discuss potential improvements to the recommendation that would make the check more palatable.

[steakhal]

I've already reviewed this change offline, thus I resign. For me this looks great, I'm eager to hear your opinions!

[futogergely]

x64 debian failed

[whisperity]

Should/does this work in C++ mode for std::whatever?

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

49

Is this ordering specific in any way? Is the rule listing them in this order? If not, can we have them listed alphabetically, for easier search and potential later change?

clang-tools-extra/docs/clang-tidy/checks/cert-msc24-c.rst

11

(And consistent capitalisation later.)

30

[futogergely]

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

49

done

clang-tools-extra/docs/clang-tidy/checks/cert-msc24-c.rst

11

done

[futogergely]

In D91000#3161296, @whisperity wrote:

Should/does this work in C++ mode for std::whatever?

Right now the checker finds the functions in the global namespace only. The recommendation is listed only in the C part of the CERT rules, and as far as I know, Annex K functions are defined in the global namespace only, (or at least based on the standard). I can't really decide if the checker should look for the functions in the std namespace as well or not...

[steakhal]

L129 and L135 are uncovered by tests. The rest of the lines are covered by tests, according to lcov.

The checker produced in total 15 reports on these 18 projects:
memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,tinyxml2,libwebm,xerces,bitcoin,protobuf,qtbase,codechecker,contour,llvm-project
You can check the reports by following this link:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?is-unique=on&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=%2aD91000-diff-4&checker-name=cert-msc24-c&diff-type=New

It seems like none of these projects actually use the annex K functions, which is not really a surprise.
VLC and lighttpd seems to use it. @futogergely could you please run your check on those projects?

clang-tools-extra/docs/clang-tidy/checks/cert-msc24-c.rst

14

remove trailing whitespaces

[aaron.ballman]

Thanks for this new check!

In terms of whether we should expose the check in C++: I think we shouldn't. Annex K *could* be supported by a C++ library implementation (http://eel.is/c++draft/library#headers-10), but none of the identifiers are added to namespace std and there's not a lot of Annex K support in C so I imagine there's even less support in C++. We can always revisit the decision later and expand support for C++ once we know what that support should look like.

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

We should also circle back with the CERT authors for updates on missing entries in their tables once we've figured out what is missing, and on the terminology concerns of referring to these as "deprecated or obsolescent" functions.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

34

I think its exclusion from the CERT page was an oversight. strlen_s() is a secure replacement for strlen() and it would be weird to leave it off the list.

41–42

This comment is not accurate. gets_s() is a secure replacement for gets().

49–59

This list appears to be missing quite a few functions with secure replacements in Annex K. For example: tmpfile_s, tmpnam_s, strerrorlen_s, strlen_s... can you check the list against the actual Annex K, as it seems the CERT recommendation is still out of date.

84–86

Fixed a few nits with the code, but gets() was never deprecated, so the message is not correct (it was present in C99 and removed in C11 with no deprecation period). I think it may be better to say "function %0 was removed in C11".

103

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h

18

The terminology used in the CERT recommendation is pretty unfortunate and I don't think we should replicate it. Many of these are *not* deprecated or obsolescent functions and calling them that will confuse users. The crux of the CERT recommendation is that these functions have better replacements in more modern versions of C. So I would probably try to focus our diagnostics and documentation around modernization rather than deprecation.

FWIW, this is feedback that should also go onto the CERT recommendation itself. I noticed someone already observed the same thing: https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions?focusedCommentId=215482395#comment-215482395

[futogergely]

"L129 and L135 are uncovered by tests. The rest of the lines are covered by tests, according to lcov."
This happens if STDC_WANT_LIB_EXT1 is defined empty (L129) or STDC_WANT_LIB_EXT1 is not literal (numeric constant, ...).

[futogergely]

"It seems like none of these projects actually use the annex K functions, which is not really a surprise.
VLC and lighttpd seems to use it. @futogergely could you please run your check on those projects?"

lighttpd: the checker issued 386 warnings. The reason is that STDC_WANT_LIB_EXT1 is defined in a header which is included everywhere. However, functions from Annex K are used only in one source file.
VLC: 2 warnings, usage of the rewind function.

Both of these projects can be compiled without Annex K as well, which makes the code cumbersome. Maybe this check can be really useful in a project where Annex K is a must.

[futogergely]

Maybe we could remove the check for setbuf() and rewind() functions, making this a pure Annex K checker. There is an overlapping with another recommendation (https://wiki.sei.cmu.edu/confluence/display/c/ERR07-C.+Prefer+functions+that+support+error+checking+over+equivalent+functions+that+don%27t), these functions are also listed there.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

41–42

If gets is removed from C11, and gets_s is introduced in C11, then gets_s cannot be a replacement or? Maybe fgets?

Also I was wondering if we would like to disable this check for C99, maybe we should remove the check for gets all together.

49–59

Missing functions added: tmpfile/tmpfile_s, tmpnam/tmpnam_s, memset/memset_s, scanf, strlen, wcslen

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h

18

Changed it to:
Checks for functions listed in CERT C Coding Standard Recommendation MSC24-C
that have safer, more secure replacements. The functions checked are considered unsafe because for
example they are missing bounds checking and/or non-reentrant. For the listed functions a
replacement function is suggested, if available. The checker heavily relies on the functions from
Annex K (Bounds-checking interfaces) of C11.

Also I changed the "is obsolescent" to "is not bounds-checking" in the getRationale function

[futogergely]

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

84–86

Done

103

Done.

[balazske]

The functions asctime and asctime_r are discouraged according to CERT MSC33-C rule. These could be added to this check as well. There is a clang SA checker SecuritySyntaxChecker that contains other obsolete functions (and the whole check looks like it can be done in clang-tidy).

[steakhal]

In D91000#3225369, @balazske wrote:

The functions asctime and asctime_r are discouraged according to CERT MSC33-C rule. These could be added to this check as well. There is a clang SA checker SecuritySyntaxChecker that contains other obsolete functions (and the whole check looks like it can be done in clang-tidy).

+1

[futogergely]

In D91000#3225369, @balazske wrote:

The functions asctime and asctime_r are discouraged according to CERT MSC33-C rule. These could be added to this check as well. There is a clang SA checker SecuritySyntaxChecker that contains other obsolete functions (and the whole check looks like it can be done in clang-tidy).

The inclusion of CERT MSC33-C rule seems to be straightforward: check for asctime and asctime_r, and suggest asctime_s if Annex K is available, otherwise suggest strftime.

security.insecureAPI: the following functions could be added to the checker: bcmp, bcopy, bzero, getpw, mktemp, vfork, and if arc4random is available: drand48, erand48, jrand48, lcong48, lrand48, mrand48, nrand48, random, rand_r.
I think for now it is enough to issue a warning of using these functions, and not suggest a replacement. Should we add an option to the checker to also check for these functions?

[martong]

I think for now it is enough to issue a warning of using these functions, and not suggest a replacement. Should we add an option to the checker to also check for these functions?

IMHO, it is okay to start with just simply issuing the warning. Later we might add that option (in a subsequent patch).

[whisperity]

Bump! Thanks @futogergely for picking up @ktomi996's work, and @steakhal for the help given during the implementation process!

In D91000#3231417, @martong wrote:

In D91000#3230055, @futogergely wrote:

I think for now it is enough to issue a warning of using these functions, and not suggest a replacement. Should we add an option to the checker to also check for these functions?

IMHO, it is okay to start with just simply issuing the warning. Later we might add that option (in a subsequent patch).

Yes, please. And I suggest indeed hiding it behind an option, e.g. ReportMoreUnsafeFunctions which I believe should default to true, but setting it to false would allow people who want their code to be "only" strict CERT-clean (that is, not care about these additional matches), can request doing so. Not offering a replacement right now is okay too, we can definitely work them in later.

In D91000#3197851, @aaron.ballman wrote:

In terms of whether we should expose the check in C++: I think we shouldn't. Annex K *could* be supported by a C++ library implementation (http://eel.is/c++draft/library#headers-10), but none of the identifiers are added to namespace std and there's not a lot of Annex K support in C so I imagine there's even less support in C++. We can always revisit the decision later and expand support for C++ once we know what that support should look like.

(Minor suggestion, but we could pull a trick with perhaps checking whether the macro is defined in the TU by the user and the library, and trying to match against the non-std:: Annex K functions, and if they are available, consider the implementation the user is running under as "AnnexK-capable-even-in-C++-mode" and start issuing warnings? This might sound very technical, and definitely for later consideration!)

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

Except for warning about gets() and suggesting fgets()?

---

Some nits: when it comes to inline comments, please mark the comments of the reviewers as "Done" when replying if you consider that comment was handled. Only you, the patch author, can do this. When there are too many open threads of comments, it gets messy to see what has been handled and what is still under discussion.

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

41–42

Yes, it's strange territory. If I make my code safer but stay pre-C11, I actually can't, because the new function isn't there yet. If I also upgrade then I'll have to make my code "safer", because the old function is now missing...

Given how brutally dangerous gets is (you very rarely see a documentation page just going Never use gets()!), I would suggest keeping at least the warning.

Although even the CERT rule mentions gets_s(). We could have some wiggle room here: do the warning for gets(), and suggest two alternatives: fgets(), or gets_s() + Annex K? fgets(stdin, ...) is also safe, if the buffer's size is given appropriately.

41–42

CERT mentions C99 TC3, which seems to be available here: https://webstore.iec.ch/p-corrigenda/isoiec9899-cor3%7Bed2.0%7Den.pdf . I'm not sure how normative this source is (iec.ch seems legit to me that this isn't just a random WGML draft!), and on page 8, in point 46 it says: "Add new paragraph after paragraph 1: The gets function is obsolescent, and is deprecated.".

This seems like nitpicking, but maybe CppReference is outdated as it never indicated the "deprecation" period?

(FYI: http://www.iso.org/standard/50510.html does not offer any purchase of the older version of the standard, or this errata.)

94

110

Maybe say this->PP or rename the parameter here ThePP and do PP = ThePP?

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h

19

21–24

(Something went wrong with the formatting here.)

42–43

clang-tools-extra/docs/ReleaseNotes.rst

95–98

Format, reflow, align with the comment in the header, etc.

[futogergely]

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

41–42

I don't use 'deprecated' in the warning message, I issue 'is insecure, and it is removed from C11' for gets instead.

I added the following replacement suggestions for gets: gets_s if Annex K is available, fgets if Annex K is not available.

49–59

strlen/strnlen_s, wcslen/wcsnlen_s, memset/memset_s, scanf/scanf_s has been added.

I did not add tmpfile/tmpfile_s, tmpnam/tmpnam_s because there is a separate CERT rule for it: https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories.

[futogergely]

I changed the class name: ObsolescentFunctionsCheck->UnsafeFunctionsCheck.
Since MSC33-C is also included, I changed the checker name to cert-msc24-msc33-c.
I added the following functions from CheckSecuritySyntaxOnly under option 'ReportMoreUnsafeFunctions': bcmp, bcopy, bzero, getpw, vfork. Since there is a replacement suggested there, I added the replacement suggestions also.
I did not add tmpnam, tmpfile, mktemp, mkstemp, rand..() to the checker, because there are separate CERT rules for these.

[balazske]

Now it would be better to have a checker called UnsafeFunctionsCheck (probably in bugprone) and add the cert checkers "msc24-c" and "msc33-c" as aliases. This makes the check extendable if more (CERT rule related or not) cases for unsafe functions are added.

[futogergely]

In D91000#3313892, @balazske wrote:

Now it would be better to have a checker called UnsafeFunctionsCheck (probably in bugprone) and add the cert checkers "msc24-c" and "msc33-c" as aliases. This makes the check extendable if more (CERT rule related or not) cases for unsafe functions are added.

Done

[futogergely]

Checker has been moved to bugprone.

[balazske]

I found only small issues.

clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp

109 ↗

(On Diff #414359)

DeclRef->getSourceRange() can be used

clang-tools-extra/docs/clang-tidy/checks/bugprone-usafe-functions.rst

4 ↗

(On Diff #414359)

I do not know if this works, it is better if this line has the same length as the line above.

105 ↗

(On Diff #414359)

These examples do not work: strcat_s and strcpy_s take different parameters than the original.

[whisperity]

Just one question if you could try this out for me: what happens if you run clang-tidy a.c b.c (two TUs in the invocation) where one of them (preferably the later one, i.e. b.c) does NOT have Annex K enabled? I believe the cached IsAnnexKAvailable (like any other TU-specific state of the check instance) should be invalidated/cleared in an overridden void onStartTranslationUnit() function.

Also, what happens if the check is run for C++ code?

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

208–214 ↗

(On Diff #421435)

What is the reason for this being a module option, when the name of the option looks like a check option? Did something change and the API requires this now? If you do Options.get("ReportMoreUnsafeFunctions", true) it will automatically work and use this default option. You also overridden the storeOptions() function properly by the looks of it, so there should be no reason for this change.

clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp

157–159 ↗

(On Diff #421435)

(And you can remove all the other // Caching the result comments.

203–212 ↗

(On Diff #421435)

Instead of wrapping the StringRef into an Optional, couldn't we achieve the same with the empty string(ref)... signalling the fact that there is no replacement?

clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h

21 ↗

(On Diff #421435)

47 ↗

(On Diff #421435)

(Superfluous comment.)

clang-tools-extra/docs/ReleaseNotes.rst

123

clang-tools-extra/docs/clang-tidy/checks/bugprone-unsafe-functions.rst

9 ↗

(On Diff #421435)

81–83 ↗

(On Diff #421435)

(Visual nit.)

[futogergely]

In D91000#3506605, @whisperity wrote:

Just one question if you could try this out for me: what happens if you run clang-tidy a.c b.c (two TUs in the invocation) where one of them (preferably the later one, i.e. b.c) does NOT have Annex K enabled? I believe the cached IsAnnexKAvailable (like any other TU-specific state of the check instance) should be invalidated/cleared in an overridden void onStartTranslationUnit() function.

Also, what happens if the check is run for C++ code?

It is working as is, a new ClangTidyCheck is created for every translation unit.

[futogergely]

updates based on comments.

[Eugene.Zelenko]

Locations for tests and check documentation was changed recently. Please rebase from main and adjust your code accordingly.

[futogergely]

In D91000#3612090, @Eugene.Zelenko wrote:

Locations for tests and check documentation was changed recently. Please rebase from main and adjust your code accordingly.

Done.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

118

Please keep checks entries in alphabetical order.

120

One sentence should be enough.

[balazske]

Another functions that can be added to the list: atoi(), atol(), atoll(), atof(). These are unsafe according to https://wiki.sei.cmu.edu/confluence/display/c/ERR34-C.+Detect+errors+when+converting+a+string+to+a+number.

[whisperity]

Generally LGTM. Please revisit the documentation and let's fix the style, and then we can land.

In D91000#3197851, @aaron.ballman wrote:

In terms of whether we should expose the check in C++: I think we shouldn't. [...]

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

@aaron.ballman I think the current version of the check satisfies these conditions. It seems the check will run for every TU, but in case there is no alternative the check could suggest, it will do nothing:

if (!ReplacementFunctionName)
  return;

Is this good enough? This seems more future-proof than juggling the LangOpts instance in yet another member function.

clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp

1 ↗

(On Diff #440259)

Ditto.

150 ↗

(On Diff #440259)

(Just so that we do not get "unused variable" warnings when compiling.)

clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h

1 ↗

(On Diff #440259)

Style nit: length of line is not padded to 80-col.

clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst

31–40 ↗

(On Diff #440259)

In general, the rendered version of the docs should be visually observed, as backtick in RST only turns on emphasis mode and not monospacing, i.e. it will be memcpy_s and not memcpy_s. Please look at the other checks and if there is a consistent style (e.g. symbol names (function) are monospaced, but options of the check is emphasised) align it to that so we have the documentation "fall into place" visually.

52 ↗

(On Diff #440259)

(Style nit: yeah this will not look like a symbol at all here)

[whisperity]

In D91000#3770071, @whisperity wrote:

In D91000#3197851, @aaron.ballman wrote:

In terms of whether we should expose the check in C++: I think we shouldn't. [...]

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

@aaron.ballman I think the current version of the check satisfies these conditions. It seems the check will run for every TU, but in case there is no alternative the check could suggest, it will do nothing:

if (!ReplacementFunctionName)
  return;

Is this good enough? This seems more future-proof than juggling the LangOpts instance in yet another member function.

@aaron.ballman @njames93 Ping!
It seems @futogergely has resigned from the company, so I'll end up flying the approach, but the one above is the last outstanding question.

[aaron.ballman]

In D91000#3770071, @whisperity wrote:

Generally LGTM. Please revisit the documentation and let's fix the style, and then we can land.

In D91000#3197851, @aaron.ballman wrote:

In terms of whether we should expose the check in C++: I think we shouldn't. [...]

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

@aaron.ballman I think the current version of the check satisfies these conditions. It seems the check will run for every TU, but in case there is no alternative the check could suggest, it will do nothing:

if (!ReplacementFunctionName)
  return;

Is this good enough? This seems more future-proof than juggling the LangOpts instance in yet another member function.

My concern with that approach was that we pay the full expense of doing the matches only get get into the check() function to bail out on all the Annex K functions, but now that there are replacements outside of Annex K, I don't see a way around paying that expense, so I think my concern has been addressed as well as it could have been.

[whisperity]

In D91000#3861942, @aaron.ballman wrote:

My concern with that approach was that we pay the full expense of doing the matches only get get into the check() function to bail out on all the Annex K functions, but now that there are replacements outside of Annex K, I don't see a way around paying that expense, so I think my concern has been addressed as well as it could have been.

I think that Clang-Tidy checks are instantiated per AST. I will look into whether we can somehow do the disabling of the check as early as possible! (In that case, we could simply NOT register the matcher related to Annex-K functions.) Either way, I'll do a rebase, re-run the tests and etc., and likely take over the check.

[futogergely]

Hi, sorry for the late answer, did not have time to check this in the last few weeks. I will try to address all of the remaining comments.

[futogergely]

Addressing review comments.

[futogergely]

In D91000#3862210, @whisperity wrote:

In D91000#3861942, @aaron.ballman wrote:

My concern with that approach was that we pay the full expense of doing the matches only get get into the check() function to bail out on all the Annex K functions, but now that there are replacements outside of Annex K, I don't see a way around paying that expense, so I think my concern has been addressed as well as it could have been.

I think that Clang-Tidy checks are instantiated per AST. I will look into whether we can somehow do the disabling of the check as early as possible! (In that case, we could simply NOT register the matcher related to Annex-K functions.) Either way, I'll do a rebase, re-run the tests and etc., and likely take over the check.

I checked, and I think that at the point of ClangTidyCheck::registerMatchers the preprocessor has not been executed yet... (and we need the value of macros STDC_LIB_EXT1 and STDC_WANT_LIB_EXT1 to decide if we need to register some matchers or not) @whisperity could you maybe double-check it please?
What we could do is:

1. add a new checker option to decide if we suggest replacements from AnnexK. We could avoid registering matchers this way, but I don't really like this, having an option for something we could decide from the defined macros.
2. As a TODO, we could make possible to register checkers AFTER the preprocessor is executed. I have not looked into this, so I don't really know if it is possible at all in the current architecture.

[whisperity]

In D91000#4020891, @futogergely wrote:

What we could do is:

1. add a new checker option to decide if we suggest replacements from AnnexK. We could avoid registering matchers this way, but I don't really like this, having an option for something we could decide from the defined macros.
2. As a TODO, we could make possible to register checkers AFTER the preprocessor is executed. I have not looked into this, so I don't really know if it is possible at all in the current architecture.

I think it's fine if we do not go a great length for changing the entire infrastructure around this. At least we looked, it's not possible.

@aaron.ballman said that we've likely did as much as we could. At least the matchers themselves aren't that expensive, it's a small string lookup (hopefully hashed or at least memoised) in a trivial search for a very specific node type only.

I'll try running at least one final grand CI test of this check just to make sure it's not crashing and such, but due to the C11 and Annex K requirement, I do not expect a lot of results...

[whisperity]

Tentative LGTM, will still need to run the "usual test projects" in the CI for confirmation. What's missing from this patch are the .rst files for the CERT-specific aliases of the check, but those are trivial and we can "add them in post" anyway.

[whisperity]

Alright, the tests came back from the CI positively and there were no crashes on like 15 projects (about 50:50 C and C++). There are some nice results:

• In LLVM, in LexPPMacroExpansion.cpp ExpandBuiltinMacro() there is a hit for a call Result = asctime(TM);. The suggestion is strftime which is available in all C++ versions and >= C99, so this seems like a valid finding. Result is a const char * which is initialised by this call.
• In Bitcoin, 2 findings in logging.cpp doing setbuf(out, nullptr);. Not sure if these are legit findings because these seem to be only "resetting" some buffering, but the warning message is legible.
• In Postgres:
  • 2 findings in isolationtester.c, same setbuf(stdout, NULL); and setbuf(stderr, NULL);. setvbuf/4 is available from C99 so this seems like a valid finding from a language version standpoint.
  • In initdb.c, a call to rewind with the comment preceding: /* now reprocess the file and store the lines */.
• In SQLite:
  • In lemon.c, a call to rewind [...]
  • In shell.c, 3 calls to rewind [...]
• In OpenSSL:
  • In randfile.c and apps.c, 2 setbuf(fileptr, NULL); calls [...]
• In Vim, 4 calls to rewind() in xxd.c, tag.c, and term.c.
  • The case in term.c is extra funny because directly following the rewind(fd) call there is a while (!feof(fd)) ...
• In MemcacheD, a setbuf(stderr, NULL); with the comment /* set stderr non-buffering ... */

It seems not many projects use Annex K at least from this test set. Note that we're primarily working on Linux, where Annex K isn't in the standard library. It has also been reported to not that useful, although it's good that the check uses them optionally. In general, we seem to have made a good enough framework for registering "unsafe standard APIs" later on with those stringswitches.

Regarding the performance, I did not see any meaningful slow-down. Most of the projects analysed in a few seconds or minutes, similar to other Tidy runs in the same infrastructure.

[whisperity]

Alright, I've done a full reanalysis after a rebase. Overhead is not meaningfully measurable, even for complex TUs such as LLVM. The check is viable in C++ code as it finds cases (such as the one described in LLVM, but also Bitcoin is a primarily C++ project), so I won't rework the check to disable it in C++ mode explicitly. It seems the name lookup is implemented pretty well, helped by the fact that the names to match are short. No crashes had been observed in the test projects; let's hope it stays the same way; the matchers themselves are simple enough. The Annex K. matcher is only registered if in >= C11 mode.
I've also gone through the discussion earlier, and it looks to me as if all the concerns were either resolved or made obsolete due to the evolution of the check.

I did make several NFC changes to the patch in post-production, such as fixing the documentation here and there, ensuring that the cert- aliases are appropriately documented, and a little bit of refactoring so internal details of the check are genuinely internal. Thus, I'll be committing a version that is different from the one here.

[whisperity]

Oddly enough, one of the buildbots caught a not matching test that was working locally... on it.

[whisperity]

Alright, right now, I have no meaningful idea why this failure appears, locally I'm trying every test command as they appear in the test, and all the tests are passing. It's as if on that system the whole Annex K support would not be allowed. Locally I added a few debug prints and I'm getting the right answers for "Whether Annex K is allowed?".

Dumping out the value:

$ clang-tidy --checks='-*,bugprone-unsafe-functions' ~/llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1

getReplacementFor(gets, 1);

And I'm getting

/home/whisperity/llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c:17:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'gets_s' should be used instead [bugprone-unsafe-functions]

And if the __STDC_ macros are defined for 0 instead of 1, the dummy output shows 0 too and the suggested replacement is fgets.

The interesting part is that builds for other platforms, such as x86_64-debian passes:

PASS: Clang Tools :: clang-tidy/checkers/bugprone/unsafe-functions.c (17639 of 68905)

---

@njames93 @aaron.ballman Do you have any idea what could make that test fail in that system?

[whisperity]

Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't find any information what x86_64-sie is...

Meanwhile, it seems my attempt at fix is not actually fixing anything, I'm trying to figure out how to at least disable the check on this specific architecture. The rest of the architectures seem to be passing normally as expected. Something must be special within Clang's default environment or compiler configuration...

[dyung]

In D91000#4099657, @whisperity wrote:

Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't find any information what x86_64-sie is...

Meanwhile, it seems my attempt at fix is not actually fixing anything, I'm trying to figure out how to at least disable the check on this specific architecture. The rest of the architectures seem to be passing normally as expected. Something must be special within Clang's default environment or compiler configuration...

Sorry responses from me might be delayed today as I am on holiday, but that build bot builds with PS4 as the default target. This is the cmake line that is used:

cmake ../llvm-project/llvm -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_BUILD_TYPE=Release -DCLANG_ENABLE_ARCMT=OFF -DCLANG_ENABLE_CLANGD=OFF -DLLVM_BUILD_RUNTIME=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4 -DLLVM_ENABLE_ASSERTIONS=ON '-DLLVM_LIT_ARGS=--verbose -j100' -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_USE_LINKER=gold '-DLLVM_ENABLE_PROJECTS=clang;cross-project-tests;llvm;clang-tools-extra;lld' -GNinja

Hopefully this can help you to reproduce the problem.

[whisperity]

In D91000#4099669, @dyung wrote:

-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4

Hopefully this can help you to reproduce the problem.

Ah, thank you very much. And indeed, giving --target=x86_64-scei-ps4 to Clang-Tidy will make the test case fail, but using x86_64-linux-gnu works perfectly.
Now knowing the exact triple I think I can delve into the preprocessor stack or something to figure out why the Annex K.-related flags, my best bet is that it's explicitly forbidden from being defined. 😉

clang-tidy /tmp/LLVM-Build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/unsafe-functions.c.tmp.c --checks='-*,bugprone-unsafe-functions' -config={} -- --target=x86_64-linux-gnu -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 -nostdinc++

My first wish is to figure out how to reliably disable the test case and have this quick-fix pushed immediately so the buildbots don't continue failing, and then we can figure out that this check should be disabled on this specific platform, or something like that...

[dyung]

In D91000#4099744, @whisperity wrote:

In D91000#4099669, @dyung wrote:

-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4

Hopefully this can help you to reproduce the problem.

Ah, thank you very much. And indeed, giving --target=x86_64-scei-ps4 to Clang-Tidy will make the test case fail, but using x86_64-linux-gnu works perfectly.
Now knowing the exact triple I think I can delve into the preprocessor stack or something to figure out why the Annex K.-related flags, my best bet is that it's explicitly forbidden from being defined. 😉

clang-tidy /tmp/LLVM-Build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/unsafe-functions.c.tmp.c --checks='-*,bugprone-unsafe-functions' -config={} -- --target=x86_64-linux-gnu -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 -nostdinc++

My first wish is to figure out how to reliably disable the test case and have this quick-fix pushed immediately so the buildbots don't continue failing, and then we can figure out that this check should be disabled on this specific platform, or something like that...

One particular oddity of our platform is that we use a different default C++ and I think C standard, so if explicitly setting the C standard causes the test to pass, that is likely the case.

To XFAIL the test, grep for lines with "XFAIL" and "ps4" and you should find some examples. It was recently changed how it worked lately.

[whisperity]

In D91000#4099773, @dyung wrote:

To XFAIL the test, grep for lines with "XFAIL" and "ps4" and you should find some examples. It was recently changed how it worked lately.

Thank you, yes I found an example. I went with UNSUPPORTED instead of XFAIL because the test implements 4 or 5 separate cases and XFAILing it all would make some cases become "Unexpectedly passed"... I did a quick commit rG9225d08ccca5be900c07eb89e907c4092bbdd462 with marking it as unsupported with a bit of an explanation in the code, and now the buildbots are coming back green.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 #include "../readability/UppercaseLiteralSuffixCheck.h"
 #include "CommandProcessorCheck.h"
 #include "DefaultOperatorNewAlignmentCheck.h"
 #include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
 #include "MutatingCopyCheck.h"
 #include "NonTrivialTypesLibcMemoryCallsCheck.h"
+#include "ObsolescentFunctionsCheck.h"
 #include "PostfixOperatorCheck.h"
 #include "ProperlySeededRandomGeneratorCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
CheckFactories.registerCheck<bugprone::SuspiciousMemoryComparisonCheck>(
         "cert-exp42-c");
     // FLP
     CheckFactories.registerCheck<FloatLoopCounter>("cert-flp30-c");
     CheckFactories.registerCheck<bugprone::SuspiciousMemoryComparisonCheck>(
         "cert-flp37-c");
     // FIO
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
     // MSC
+    CheckFactories.registerCheck<ObsolescentFunctionsCheck>("cert-msc24-c");
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc32-c");
     // POS
     CheckFactories.registerCheck<bugprone::BadSignalToKillThreadCheck>(
         "cert-pos44-c");
     CheckFactories
         .registerCheck<concurrency::ThreadCanceltypeAsynchronousCheck>(

clang-tools-extra/clang-tidy/cert/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   FrontendOpenMP
   )
 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   CommandProcessorCheck.cpp
   DefaultOperatorNewAlignmentCheck.cpp
   DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
   MutatingCopyCheck.cpp
   NonTrivialTypesLibcMemoryCallsCheck.cpp
+  ObsolescentFunctionsCheck.cpp
   PostfixOperatorCheck.cpp
   ProperlySeededRandomGeneratorCheck.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h

+//===--- ObsolescentFunctionsCheck.h - clang-tidy ---------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_OBSOLESCENTFUNCTIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_OBSOLESCENTFUNCTIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Checks for deprecated and obsolescent function listed in
+/// CERT C Coding Standard Recommendation MSC24 - C. For the listed functions,
+/// an alternative, safe replacement is suggested if available.
+/// The checker heavily relies on the availability of annexK(Bounds - checking
+/// interfaces) from C11. For the user-facing documentation see:
+///
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc24-c.html
+class ObsolescentFunctionsCheck : public ClangTidyCheck {
+public:
+  ObsolescentFunctionsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+
+private:
+  bool useSafeFunctionsFromAnnexK() const;
+  Optional<std::string>
+  getReplacementFunctionNameFor(StringRef FunctionName,
+                                bool FunctionHasAnnexKReplacement) const;
+
+  Preprocessor *PP = nullptr;
+
+  // Used for caching the result of useSafeFunctionsFromAnnexK.
+  mutable llvm::Optional<bool> IsAnnexKAvailable;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_OBSOLESCENTFUNCTIONSCHECK_H

clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp

+//===--- ObsolescentFunctionsCheck.cpp - clang-tidy -----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ObsolescentFunctionsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+#include <cassert>
+
+using namespace clang::ast_matchers;
+using namespace llvm;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+static constexpr llvm::StringLiteral DeprecatedFunctionNamesId =
+    "DeprecatedFunctionNamesId";
+static constexpr llvm::StringLiteral FunctionNamesId = "FunctionNamesId";
+static constexpr llvm::StringLiteral FunctionNamesWithAnnexKReplacementId =
+    "FunctionNamesWithAnnexKReplacementExpr";
+
+static constexpr llvm::StringLiteral DeclRefId = "DeclRefId";
+
+static StringRef getRationale(StringRef FunctionName) {
+  return StringSwitch<StringRef>(FunctionName)
+      .Cases("asctime", "ctime", "is non-reentrant")
+      .Cases("fopen", "freopen", "has no exclusive access to file")
+      .Cases("rewind", "setbuf", "has no error detection")
+      .Default("is obsolescent");
+}
+
+void ObsolescentFunctionsCheck::registerMatchers(MatchFinder *Finder) {
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+
+  // Matching functions with replacements without Annex K.
+  auto FunctionNamesMatcher = hasAnyName("::rewind", "::setbuf");
+
+  // Matching functions with safe replacements in Annex K.
+  auto FunctionNamesWithAnnexKReplacementMatcher = hasAnyName(
+      "::asctime", "::bsearch", "::ctime", "::fopen", "::fprintf", "::freopen",
+      "::fscanf", "::fwprintf", "::fwscanf", "::getenv", "::gmtime",
+      "::localtime", "::mbsrtowcs", "::mbstowcs", "::memcpy", "::memmove",
+      "::printf", "::qsort", "::snprintf", "::sprintf", "::sscanf", "::strcat",
+      "::strcpy", "::strerror", "::strncat", "::strncpy", "::strtok",
+      "::swprintf", "::swscanf", "::vfprintf", "::vfscanf", "::vfwprintf",
+      "::vfwscanf", "::vprintf", "::vscanf", "::vsnprintf", "::vsprintf",
+      "::vsscanf", "::vswprintf", "::vswscanf", "::vwprintf", "::vwscanf",
+      "::wcrtomb", "::wcscat", "::wcscpy", "::wcsncat", "::wcsncpy",
+      "::wcsrtombs", "::wcstok", "::wcstombs", "::wctomb", "::wmemcpy",
+      "::wmemmove", "::wprintf", "::wscanf");
+
+  Finder->addMatcher(
+      declRefExpr(
+          to(anyOf(functionDecl(DeprecatedFunctionNamesMatcher)
+                       .bind(DeprecatedFunctionNamesId),
+                   functionDecl(FunctionNamesMatcher).bind(FunctionNamesId),
+                   functionDecl(FunctionNamesWithAnnexKReplacementMatcher)
+                       .bind(FunctionNamesWithAnnexKReplacementId))))
+          .bind(DeclRefId),
+      this);
+}
+
+void ObsolescentFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>(DeclRefId);
+  assert(DeclRef && "No matching declaration reference found.");
+  SourceRange Range = SourceRange(DeclRef->getBeginLoc(), DeclRef->getEndLoc());
+
+  // FIXME: I'm not sure if this can ever happen, but to be on the safe side,
+  // validity is checked.
+  if (Range.isInvalid())
+    return;
+
+  if (const auto *Deprecated =
+          Result.Nodes.getNodeAs<FunctionDecl>(DeprecatedFunctionNamesId)) {
+    diag(Range.getBegin(),
+         "function '%0' is deprecated as of C99, removed from C11.")
+        << Deprecated->getName() << Range;
+    return;
+  }
+
+  const auto *Normal = Result.Nodes.getNodeAs<FunctionDecl>(FunctionNamesId);
+  const auto *AnnexK = Normal ? nullptr
+                              : Result.Nodes.getNodeAs<FunctionDecl>(
+                                    FunctionNamesWithAnnexKReplacementId);
+  assert((Normal && !AnnexK) || (!Normal && AnnexK));
+
+  StringRef FunctionName = (Normal ? Normal : AnnexK)->getName();
+  Optional<std::string> ReplacementFunctionName =
+      getReplacementFunctionNameFor(FunctionName, AnnexK);
+
+  if (!ReplacementFunctionName.hasValue())
+    return;
+
+  diag(Range.getBegin(), "function '%0' %1; '%2' should be used instead.")
+      << FunctionName << getRationale(FunctionName)
+      << ReplacementFunctionName.getValue() << Range;
+}
+
+void ObsolescentFunctionsCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  ObsolescentFunctionsCheck::PP = PP;
+}
+
+bool ObsolescentFunctionsCheck::useSafeFunctionsFromAnnexK() const {
+  if (IsAnnexKAvailable.hasValue())
+    return IsAnnexKAvailable.getValue();
+
+  assert(PP && "No Preprocessor registered.");
+
+  if (!PP->isMacroDefined("__STDC_LIB_EXT1__") ||
+      !PP->isMacroDefined("__STDC_WANT_LIB_EXT1__")) {
+    // Caching the result.
+    return (IsAnnexKAvailable = false).getValue();
+  }
+
+  const auto *MI =
+      PP->getMacroInfo(PP->getIdentifierInfo("__STDC_WANT_LIB_EXT1__"));
+  if (!MI || MI->tokens_empty()) {
+    // Caching the result.
+    return (IsAnnexKAvailable = false).getValue();
+  }
+
+  const Token &T = MI->tokens().back();
+  if (!T.isLiteral() || !T.getLiteralData()) {
+    // Caching the result.
+    return (IsAnnexKAvailable = false).getValue();
+  }
+
+  // Caching the result.
+  IsAnnexKAvailable = StringRef(T.getLiteralData(), T.getLength()) == "1";
+  return IsAnnexKAvailable.getValue();
+}
+
+Optional<std::string> ObsolescentFunctionsCheck::getReplacementFunctionNameFor(
+    StringRef FunctionName, bool FunctionHasAnnexKReplacement) const {
+  if (FunctionHasAnnexKReplacement && useSafeFunctionsFromAnnexK())
+    return (Twine{FunctionName} + "_s").str();
+
+  return StringSwitch<Optional<std::string>>(FunctionName)
+      .Case("rewind", Optional<std::string>{"fseek"})
+      .Case("setbuf", Optional<std::string>{"setvbuf"})
+      .Default(None);
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`abseil-cleanup-ctad
   deduction (CTAD), in C++17 and higher.
 
 - New :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` check.
 
   Finds potentially incorrect calls to ``memcmp()`` based on properties of the
   arguments.
 
+- New :doc:`cert-msc24-c
+  <clang-tidy/checks/cert-msc24-c>` check.
+
+  Checks for deprecated and obsolescent functions listed in
+  CERT C Coding Standard Recommendation MSC24-C. For the listed functions,
+  an alternative, more secure replacement is suggested, if available. The checker heavily
+  relies on the functions from Annex K (Bounds-checking interfaces) of C11.
+
 - New :doc:`cppcoreguidelines-virtual-class-destructor
   <clang-tidy/checks/cppcoreguidelines-virtual-class-destructor>` check.
 
   Finds virtual classes whose destructor is neither public and virtual nor
   protected and non-virtual.
 
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.
 
   Reports identifiers whose names are too short. Currently checks local
   variables and function parameters only.
 
 
 - New :doc:`readability-data-pointer <clang-tidy/checks/readability-data-pointer>` check.
 
   Finds cases where code could use ``data()`` rather than the address of the
   element at index 0 in a container.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-err33-c
   <clang-tidy/checks/cert-err33-c>` to
   :doc:`bugprone-unused-return-value
   <clang-tidy/checks/bugprone-unused-return-value>` was added.
 
 - New alias :doc:`cert-exp42-c
   <clang-tidy/checks/cert-exp42-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 - New alias :doc:`cert-flp37-c

clang-tools-extra/docs/clang-tidy/checks/cert-msc24-c.rst

+.. title:: clang-tidy - cert-msc24-c
+
+cert-msc24-c
+============
+
+Checks for deprecated and obsolescent functions listed in
+CERT C Coding Standard Recommendation MSC24-C (`MSC24-C. Do not use deprecated or obsolescent functions
+<https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions>`_.).
+For the listed functions, an alternative, more secure replacement is suggested, if available.
+The checker heavily relies on the functions from Annex K (Bounds-checking interfaces) of C11.
+
+For the following functions, replacement is suggested from Annex K: `asctime`, 
+`ctime`, `fopen`, `freopen`, `bsearch`, `fprintf`, `fscanf`, `fwprintf`, `fwscanf`, 
+`getenv`, `gmtime`, `localtime`, `mbsrtowcs`, `mbstowcs`, `memcpy`, `memmove`, `printf`, 
+`qsort`, `snprintf`, `sprintf`,  `sscanf`, `strcat`, `strcpy`, `strerror`, 
+`strncat`, `strncpy`, `strtok`, `swprintf`, `swscanf`, `vfprintf`, `vfscanf`, `vfwprintf`, 
+`vfwscanf`, `vprintf`, `vscanf`, `vsnprintf`, `vsprintf`, `vsscanf`, `vswprintf`, 
+`vswscanf`, `vwprintf`, `vwscanf`, `wcrtomb`, `wcscat`, `wcscpy`, `wcsncat`, `wcsncpy`, 
+`wcsrtombs`, `wcstok`, `wcstombs`, `wctomb`, `wmemcpy`, `wmemmove`, `wprintf`, `wscanf`. 
+If Annex K is not available, the checker ignores these functions.
+
+The availability of Annex K is checked based on the following macros.
+ - `__STDC_LIB_EXT1__`: feature macro, which indicates the presence of
+   Annex K (Bounds-checking interfaces) in the library implementation
+ - `__STDC_WANT_LIB_EXT1__`: user defined macro, which indicates if the user wants the functions from
+   Annex K to be defined.
+
+Both macros have to be defined to suggest replacement functions from Annex K. `__STDC_LIB_EXT1__` is
+defined by the library implementation, and `__STDC_WANT_LIB_EXT1__` must be defined to "1" by the user
+before including any system headers.
+
+Deprecated function `gets` is checked, and a warning is issued if used.
+
+The following functions are also checked, and an alternative replacement function is suggested:
+ - `rewind`, suggested replacement: `fseek`
+ - `setbuf`, suggested replacement: `setvbuf`
+
+The following functions are covered in the check `cert-err34-c <cert-err34-c.html>`_,
+so this checker **ignores** them: `atof`, `atoi`, `atol`, `atoll`.
+
+Examples:
+
+.. code-block:: c++
+  
+  //__STDC_LIB_EXT1__ is defined by the library implementation
+  #define __STDC_WANT_LIB_EXT1__ 1
+
+  #include <string.h> // defines the functions from Annex K
+  #include <stdio.h>
+  
+  enum { BUFSIZE = 32 };
+
+  void fWarning(const char *msg) { 
+    static const char prefix[] = "Error: ";
+    static const char suffix[] = "\n";
+    char buf[BUFSIZE] = {0}; 
+    
+    strcpy(buf, prefix); // warning: function 'strcpy' is obsolescent; 'strcpy_s' should be used instead.
+    strcat(buf, msg); // warning: function 'strcat' is obsolescent; 'strcat_s' should be used instead.
+    strcat(buf, suffix); // warning: function 'strcat' is obsolescent; 'strcat_s' should be used instead.
+    if (fputs(buf, stderr) < 0) {
+      // error handling
+      return;
+    }
+  }
+
+  void fUsingSafeFunctions(const char *msg) { 
+    static const char prefix[] = "Error: ";
+    static const char suffix[] = "\n";
+    char buf[BUFSIZE] = {0}; 
+    
+    if (strcpy_s(buf, prefix) != 0) {
+      // error handling
+      return;
+    }
+
+    if (strcat_s(buf, msg) != 0) {
+      // error handling
+      return;
+    }
+    
+    if (strcat_s(buf, suffix) != 0) {
+      // error handling
+      return;
+    }
+    
+    if (fputs(buf, stderr) < 0) {
+      // error handling
+      return;
+    }
+  }

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `cert-err34-c <cert-err34-c.html>`_,
    `cert-err52-cpp <cert-err52-cpp.html>`_,
    `cert-err58-cpp <cert-err58-cpp.html>`_,
    `cert-err60-cpp <cert-err60-cpp.html>`_,
    `cert-exp42-c <cert-exp42-c.html>`_,
    `cert-flp30-c <cert-flp30-c.html>`_,
    `cert-flp37-c <cert-flp37-c.html>`_,
    `cert-mem57-cpp <cert-mem57-cpp.html>`_,
+   `cert-msc24-c <cert-msc24-c.html>`_,
    `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `cert-oop57-cpp <cert-oop57-cpp.html>`_,
    `cert-oop58-cpp <cert-oop58-cpp.html>`_,
    `clang-analyzer-core.DynamicTypePropagation <clang-analyzer-core.DynamicTypePropagation.html>`_,
    `clang-analyzer-core.uninitialized.CapturedBlockVariable <clang-analyzer-core.uninitialized.CapturedBlockVariable.html>`_,
    `clang-analyzer-cplusplus.InnerPointer <clang-analyzer-cplusplus.InnerPointer.html>`_,
    `clang-analyzer-nullability.NullableReturnedFromNonnull <clang-analyzer-nullability.NullableReturnedFromNonnull.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cert-msc24-c.c

+// RUN: %check_clang_tidy -check-suffix=WITH-ANNEX-K    %s cert-msc24-c %t -- -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s cert-msc24-c %t -- -- -U__STDC_LIB_EXT1__   -U__STDC_WANT_LIB_EXT1__
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s cert-msc24-c %t -- -- -D__STDC_LIB_EXT1__=1 -U__STDC_WANT_LIB_EXT1__
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s cert-msc24-c %t -- -- -U__STDC_LIB_EXT1__   -D__STDC_WANT_LIB_EXT1__=1
+
+typedef void *FILE;
+char *gets(char *s);
+void rewind(FILE *stream);
+void setbuf(FILE *stream, char *buf);
+
+void f1(char *s, FILE *f) {
+  gets(s);
+  // CHECK-MESSAGES-WITH-ANNEX-K:    :[[@LINE-1]]:3: warning: function 'gets' is deprecated as of C99, removed from C11.
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'gets' is deprecated as of C99, removed from C11.
+
+  rewind(f);
+  // CHECK-MESSAGES-WITH-ANNEX-K:    :[[@LINE-1]]:3: warning: function 'rewind' has no error detection; 'fseek' should be used instead.
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'rewind' has no error detection; 'fseek' should be used instead.
+
+  setbuf(f, s);
+  // CHECK-MESSAGES-WITH-ANNEX-K:    :[[@LINE-1]]:3: warning: function 'setbuf' has no error detection; 'setvbuf' should be used instead.
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'setbuf' has no error detection; 'setvbuf' should be used instead.
+}
+
+struct tm;
+char *asctime(const struct tm *timeptr);
+
+void f2(const struct tm *timeptr) {
+  asctime(timeptr);
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'asctime' is non-reentrant; 'asctime_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+
+  char *(*f_ptr1)(const struct tm *) = asctime;
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:40: warning: function 'asctime' is non-reentrant; 'asctime_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+
+  char *(*f_ptr2)(const struct tm *) = &asctime;
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:41: warning: function 'asctime' is non-reentrant; 'asctime_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+}
+
+FILE *fopen(const char *filename, const char *mode);
+FILE *freopen(const char *filename, const char *mode, FILE *stream);
+int fscanf(FILE *stream, const char *format, ...);
+
+void f3(char *s, FILE *f) {
+  fopen(s, s);
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'fopen' has no exclusive access to file; 'fopen_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+
+  freopen(s, s, f);
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'freopen' has no exclusive access to file; 'freopen_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+
+  int i;
+  fscanf(f, "%d", &i);
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'fscanf' is obsolescent; 'fscanf_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+}
+
+typedef int time_t;
+char *ctime(const time_t *timer);
+
+void f4(const time_t *timer) {
+  ctime(timer);
+  // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'ctime' is non-reentrant; 'ctime_s' should be used instead.
+  // no-warning WITHOUT-ANNEX-K
+}
+
+typedef int errno_t;
+typedef __SIZE_TYPE__ size_t;
+typedef size_t rsize_t;
+errno_t asctime_s(char *s, rsize_t maxsize, const struct tm *timeptr);
+errno_t strcat_s(char *s1, rsize_t s1max, const char *s2);
+int fseek(FILE *stream, long int offset, int whence);
+int setvbuf(FILE *stream, char *buf, int mode, size_t size);
+
+void fUsingSafeFunctions(const struct tm *timeptr, FILE *f) {
+  const size_t BUFFSIZE = 32;
+  char buf[BUFFSIZE] = {0};
+
+  // no-warning, safe function from annex K is used
+  if (asctime_s(buf, BUFFSIZE, timeptr) != 0)
+    return;
+
+  // no-warning, safe function from annex K is used
+  if (strcat_s(buf, BUFFSIZE, "something") != 0)
+    return;
+
+  // no-warning, fseeks supports error checking
+  if (fseek(f, 0, 0) != 0)
+    return;
+
+  // no-warning, setvbuf supports error checking
+  if (setvbuf(f, buf, 0, BUFFSIZE) != 0)
+    return;
+}