[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

[Koller Tamás via Phabricator via cfe-commits]

ktomi996 added a comment.

In D91000#2382562 <https://reviews.llvm.org/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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000