[Clang-tidy] CERT-DCL58-CPP (checker for std namespace modification)

[xazax.hun]

Summary

This checker warns for the modification of std namespace.

More information:
https://www.securecoding.cert.org/confluence/display/cplusplus/DCL58-CPP.+Do+not+modify+the+standard+namespaces

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/cert-msc53-cpp.rst

6 ↗

(On Diff #67734)

Please highlight std and posix with ``.

[aaron.ballman]

Thank you for working on this check! There is a slight problem, however, in that the check as-written will flag false positives because there are times when it is permissible to modify the std namespace. The important bit of the CERT requirement is "Do not add declarations or definitions to the standard namespaces std or posix, or to a namespace contained therein, except for a template specialization that depends on a user-defined type that meets the standard library requirements for the original template." So the part that's missing from this check is excluding template specializations that depend on user-defined types. For instance, the following should be valid:

namespace std {
template <>
struct is_error_code_enum<llvm::object::object_error> : std::true_type {};
}

(This comes from Error.h in LLVM.)

clang-tidy/cert/CERTTidyModule.cpp

37–38 ↗

(On Diff #67734)

Should put this under a // MSC comment rather than the // DCL one.

clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

33 ↗

(On Diff #67734)

Should use format specifiers instead of building the string manually. e.g., using %0. Also, diagnostics should start with a lowercase letter and "to" should be "in".

clang-tidy/cert/DontModifyStdNamespaceCheck.h

19 ↗

(On Diff #67734)

s/to/in

docs/clang-tidy/checks/list.rst

12 ↗

(On Diff #67734)

Should keep this in alphabetical order.

[hokein]

clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

20 ↗

(On Diff #67734)

This check should be only available in cpp code.

if (!getLangOpts().CPlusPlus)
  return;

docs/clang-tidy/checks/cert-msc53-cpp.rst

4 ↗

(On Diff #67734)

=== should be the same with cert-msc53-cpp.

6 ↗

(On Diff #67734)

Adding an example explaining the check would make the doc more clear. An example is worth a thousand words :)

test/clang-tidy/cert-dont-modify-std-namespace.cpp

1 ↗

(On Diff #67734)

Do we need c++1z flag here?

[xazax.hun]

test/clang-tidy/cert-dont-modify-std-namespace.cpp

1 ↗

(On Diff #67734)

I think it is there for the following code snippet:

namespace posix::a {

[xazax.hun]

Benedek, do you have time to address the review comments or do you want me to commandeer this revision?

[falho]

Unfortunately I wont have time to work on this check anymore... thank you for understanding!

[xazax.hun]

• Updated to latest trunk.
• The cert rule was renamed, the patch is updated accordingly.
• Fixes as per review comments.

[aaron.ballman]

clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

34 ↗

(On Diff #88333)

I think this will still trigger false positives because there are times when it is permissible to modify the std namespace outside of a system header. The important bit of the CERT requirement is "Do not add declarations or definitions to the standard namespaces std or posix, or to a namespace contained therein, except for a template specialization that depends on a user-defined type that meets the standard library requirements for the original template." So the part that's missing from this check is excluding template specializations that depend on user-defined types. For instance, the following should be valid:

namespace std {
template <>
struct is_error_code_enum<llvm::object::object_error> : std::true_type {};
}

(This comes from Error.h in LLVM.)

36 ↗

(On Diff #88333)

No need to call getName() -- you can pass N directly. When you correct this, also remove the explicit quotes from the diagnostic (they're added automatically by the diagnostics engine).

docs/clang-tidy/checks/cert-dcl58-cpp.rst

8 ↗

(On Diff #88333)

You should add a link to the CERT guideline this addresses. See docs/clang-tidy/checks/cert-dcl50-cpp.rst as an example.

[xazax.hun]

• Add a simulated system header for tests.
• Do not warn on explicit template specializations within std namespace.
• Do not warn when the namespace is empty (so no declaration is introduced within the namespace)
• Extended the test cases.
• Added a link to the documentation
• Updated the name of the checker in the description and title of this revision.

[aaron.ballman]

clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

28 ↗

(On Diff #88506)

I think this is missing function declarations that are explicit template specializations as well (a common instance of this is specializing std::swap).

test/clang-tidy/cert-dcl58-cpp.cpp

60 ↗

(On Diff #88506)

I'd like to see a test where the user specializes a function template, like swap() to make sure we don't diagnose on that.

[xazax.hun]

• Do not warn for function specializations within the std namespace.
• Add a test case for swap.

[aaron.ballman]

LGTM, thank you for working on this!

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

 #include "../ClangTidyModuleRegistry.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/MoveConstructorInitCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "CommandProcessorCheck.h"
+#include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
 
 class CERTModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // C++ checkers
     // DCL
     CheckFactories.registerCheck<VariadicFunctionDefCheck>("cert-dcl50-cpp");
     CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
+    CheckFactories.registerCheck<DontModifyStdNamespaceCheck>(
+        "cert-dcl58-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
         "cert-dcl59-cpp");
     // OOP
     CheckFactories.registerCheck<misc::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
     // ERR
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err09-cpp");

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   CommandProcessorCheck.cpp
+  DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 

clang-tools-extra/trunk/clang-tidy/cert/DontModifyStdNamespaceCheck.h

+//===--- DontModifyStdNamespaceCheck.h - clang-tidy--------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_DONT_MODIFY_STD_NAMESPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_DONT_MODIFY_STD_NAMESPACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Modification of the std or posix namespace can result in undefined behavior.
+/// This check warns for such modifications.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc53-cpp.html
+class DontModifyStdNamespaceCheck : public ClangTidyCheck {
+public:
+  DontModifyStdNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_DONT_MODIFY_STD_NAMESPACE_H

clang-tools-extra/trunk/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

+//===--- DontModifyStdNamespaceCheck.cpp - clang-tidy----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DontModifyStdNamespaceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      namespaceDecl(unless(isExpansionInSystemHeader()),
+                    anyOf(hasName("std"), hasName("posix")),
+                    has(decl(unless(anyOf(
+                        functionDecl(isExplicitTemplateSpecialization()),
+                        cxxRecordDecl(isExplicitTemplateSpecialization()))))))
+          .bind("nmspc"),
+      this);
+}
+
+void DontModifyStdNamespaceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *N = Result.Nodes.getNodeAs<NamespaceDecl>("nmspc");
+
+  // Only consider top level namespaces.
+  if (N->getParent() != Result.Context->getTranslationUnitDecl())
+    return;
+
+  diag(N->getLocation(),
+       "modification of %0 namespace can result in undefined behavior")
+      << N;
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New `cert-dcl58-cpp
+  <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl58-cpp.html>`_ check
+
+  Finds modification of the ``std`` or ``posix`` namespace.
+
 - New `readability-misleading-indentation
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
 
   Finds misleading indentation where braces should be introduced or the code should be reformatted.
 
 - New `safety-no-assembler
   <http://clang.llvm.org/extra/clang-tidy/checks/safety-no-assembler.html>`_ check
 

clang-tools-extra/trunk/docs/clang-tidy/checks/cert-dcl58-cpp.rst

+.. title:: clang-tidy - cert-dcl58-cpp
+
+cert-dcl58-cpp
+==============
+
+Modification of the ``std`` or ``posix`` namespace can result in undefined
+behavior.
+This check warns for such modifications.
+
+Examples:
+
+.. code-block:: c++
+
+  namespace std {
+    int x; // May cause undefined behavior.
+  }
+
+
+This check corresponds to the CERT C++ Coding Standard rule
+`DCL58-CPP. Do not modify the standard namespaces
+<https://www.securecoding.cert.org/confluence/display/cplusplus/DCL58-CPP.+Do+not+modify+the+standard+namespaces>`_.

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

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
+   cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err09-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err09-cpp>
    cert-err34-c
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>

clang-tools-extra/trunk/test/clang-tidy/Inputs/Headers/system-header-simulation.h

+#pragma clang system_header
+
+namespace std {
+
+template<class T, T v>
+struct integral_constant {
+    static constexpr T value = v;
+    typedef T value_type;
+    typedef integral_constant type;
+    constexpr operator value_type() const noexcept { return value; }
+};
+
+template <bool B>
+using bool_constant = integral_constant<bool, B>;
+using true_type = bool_constant<true>;
+using false_type = bool_constant<false>;
+
+template<class T>
+struct is_error_code_enum : false_type {};
+
+template<class T>
+void swap(T &a, T &b);
+}
+

clang-tools-extra/trunk/test/clang-tidy/cert-dcl58-cpp.cpp

+// RUN: %check_clang_tidy %s cert-dcl58-cpp %t -- -- -std=c++1z -I %S/Inputs/Headers
+
+#include "system-header-simulation.h"
+
+namespace A {
+  namespace B {
+    int b;
+  }
+}
+
+namespace A {
+  namespace B {
+    int c;
+  }
+}
+
+namespace posix {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'posix' namespace can result in undefined behavior [cert-dcl58-cpp]
+  namespace vmi {
+  }
+}
+
+namespace std {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'std' namespace can
+  int stdInt;
+}
+
+namespace foobar {
+  namespace std {
+    int bar;
+  }
+}
+
+namespace posix::a {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'posix' namespace 
+}
+
+enum class MyError {
+  ErrorA,
+  ErrorB
+};
+
+namespace std {
+template <>
+struct is_error_code_enum<MyError> : std::true_type {};
+
+template<>
+void swap<MyError>(MyError &a, MyError &b);
+}
+
+enum class MyError2 {
+  Error2A,
+  Error2B
+};
+
+namespace std {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'std' namespace 
+template <>
+struct is_error_code_enum<MyError2> : std::true_type {};
+
+int foobar;
+}
+
+using namespace std;
+
+int x;
+