[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 , @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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91000: MSC24-C Obsolescent Functions check

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

ktomi996 created this revision.
ktomi996 added reviewers: aaron.ballman, alexfh, hokein, njames93.
ktomi996 added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.
ktomi996 requested review of this revision.

This checker guards against using some vulnerable C functions which are 
mentioned in MSC24-C in obsolescent functions table.
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.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91000

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/CountFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/misc/CountFunctionsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp

Index: clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cert-obsolescent-functions %t --
+
+#define __STDC_LIB_EXT1__
+#define __STDC_WANT_LIB_EXT1__ 1
+void * memmove(void *, void *, unsigned int);
+void f1(const char *in) {
+  int i = 1;
+  int j = 2;
+  memmove(&i, &j, sizeof(int));
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unsafe function memmove used. Safe memmove_s can be used instead. [cert-obsolescent-functions]
+}
+
+
+void f2(const char *in) {
+void * (*func_ptr)(void *, void *, unsigned int) = memmove;
+// CHECK-MESSAGES: :[[@LINE-1]]:56: warning: Unsafe function memmove used. Safe memmove_s can be used instead. [cert-obsolescent-functions]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -100,6 +100,7 @@
cert-msc32-c (redirects to cert-msc51-cpp) 
cert-msc50-cpp
cert-msc51-cpp
+   cert-obsolescent-functions
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) 
Index: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
@@ -0,0 +1,23 @@
+..title::clang-tidy-cert-obsolescent-functions
+
+cert-obsolescent-functions
+==
+
+Guards against using some unsafe function calls and function pointers which initialized with unsafe functions if __STDC_LIB_EXT1__ macro is defined and the value of __STDC_WANT_LIB_EXT1__ is 1. The usage of following functions are checked : bsearch,
+fprintf, fscanf, fwprintf, fwscanf, getenv, gmtime, localtime, mbsrtowcs,
+mbstowcs, memcpy, memmove, printf, qsort, setbuf, snprintf, sprintf, sscanf,
+strcat, strcpy, strerror, strncat, strncpy, strtok, swprintf, swscanf,
+vfprintf, vfscanf, vfwprintf, vfwscanf, vprintf, vscanf vsnprintf, vspr,
+wcscpy, wcsncat, wcsncpy wcsrtombs, wcstok, wcstombs, wctomb, wmemcpy,
+wmemmove, wprintf, wscanf,
+strlen
+
+This is a CERT security rule:
+https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions
+  Example :
+  .code-block::
+#define __STDC_WANT_LIB_EXT1__ 1
+int i = 2;
+int j = 3;
+memmove(&i, &j, sizeof(int)); // diagnosed if __STDC_LIB_EXT1__ is defined in a
+// header, memmove_s usage is suggested instead.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -194,6 +194,13 @@
   against self-assignment either by checking self-assignment explicitly or
   using the copy-and-swap or the copy-and-move method.
 
+- New :doc:`cert-obsolescent-functions
+  ` check.
+
+  Guards against using some unsafe function calls and function pointers which
+  initialized with unsafe functions if some macros defined.
+
+
 - New :doc:`fuchsia-default-arguments-calls
   ` check

[PATCH] D91000: MSC24-C Obsolescent Functions check

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

ktomi996 updated this revision to Diff 303633.

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

https://reviews.llvm.org/D91000

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp

Index: clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cert-obsolescent-functions %t --
+
+#define __STDC_LIB_EXT1__
+#define __STDC_WANT_LIB_EXT1__ 1
+void * memmove(void *, void *, unsigned int);
+void f1(const char *in) {
+  int i = 1;
+  int j = 2;
+  memmove(&i, &j, sizeof(int));
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Unsafe function memmove used. Safe memmove_s can be used instead. [cert-obsolescent-functions]
+}
+
+
+void f2(const char *in) {
+void * (*func_ptr)(void *, void *, unsigned int) = memmove;
+// CHECK-MESSAGES: :[[@LINE-1]]:56: warning: Unsafe function memmove used. Safe memmove_s can be used instead. [cert-obsolescent-functions]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -100,6 +100,7 @@
cert-msc32-c (redirects to cert-msc51-cpp) 
cert-msc50-cpp
cert-msc51-cpp
+   cert-obsolescent-functions
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) 
Index: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst
@@ -0,0 +1,23 @@
+..title::clang-tidy-cert-obsolescent-functions
+
+cert-obsolescent-functions
+==
+
+Guards against using some unsafe function calls and function pointers which initialized with unsafe functions if __STDC_LIB_EXT1__ macro is defined and the value of __STDC_WANT_LIB_EXT1__ is 1. The usage of following functions are checked : bsearch,
+fprintf, fscanf, fwprintf, fwscanf, getenv, gmtime, localtime, mbsrtowcs,
+mbstowcs, memcpy, memmove, printf, qsort, setbuf, snprintf, sprintf, sscanf,
+strcat, strcpy, strerror, strncat, strncpy, strtok, swprintf, swscanf,
+vfprintf, vfscanf, vfwprintf, vfwscanf, vprintf, vscanf vsnprintf, vspr,
+wcscpy, wcsncat, wcsncpy wcsrtombs, wcstok, wcstombs, wctomb, wmemcpy,
+wmemmove, wprintf, wscanf,
+strlen
+
+This is a CERT security rule:
+https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions
+  Example :
+  .code-block::
+#define __STDC_WANT_LIB_EXT1__ 1
+int i = 2;
+int j = 3;
+memmove(&i, &j, sizeof(int)); // diagnosed if __STDC_LIB_EXT1__ is defined in a
+// header, memmove_s usage is suggested instead.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -194,6 +194,13 @@
   against self-assignment either by checking self-assignment explicitly or
   using the copy-and-swap or the copy-and-move method.
 
+- New :doc:`cert-obsolescent-functions
+  ` check.
+
+  Guards against using some unsafe function calls and function pointers which
+  initialized with unsafe functions if some macros defined.
+
+
 - New :doc:`fuchsia-default-arguments-calls
   ` check.
 
Index: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h
@@ -0,0 +1,36 @@
+//===--- 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"
+n