[clang-tidy] Adding cert-pos34-c check

[zukatsinadze]

Summary

According to
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
cert-pos34-c check is created. The check warns if putenv function is
called with automatic storage variable as an argument.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

200

Please use double back-ticks to highlight putenv.

[Charusso]

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

29

alloc -> alloca

35

It is not a Decl, but a CallExpr, maybe use PutenvCall?

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

20

Extra space.

35

No new-line.

clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp

60

clang-format?

[zukatsinadze]

changes after review.

[zukatsinadze]

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

29

I think `alloca` allocates memory on stack, so thats why I didn't include it here.

[aaron.ballman]

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

27

I don't know that this is sufficient for the check, and I sort of think this may need to be implemented by the static analyzer rather than clang-tidy. The initialization of the variable is going to be control flow sensitive. Consider something like:

void foo(void) {
  char *buffer = "huttah!";
  if (rand() % 2 == 0) {
    buffer = malloc(5);
    strcpy(buffer, "woot");
  }
  putenv(buffer);
}

void bar(void) {
  char *buffer = malloc(5);
  strcpy(buffer, "woot");

  if (rand() % 2 == 0) {
    free(buffer);
    buffer = "blah blah blah";
  }
  putenv(buffer);
}

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

4

Underlining looks incorrect here.

6

Finds calls to the `putenv` function which pass a pointer to an automatic variable as the argument.

23

CERT Standard -> CERT C Coding Standard

[zukatsinadze]

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

27

Yes, I see your point. I will try to rewrite it as SA checker.
Thanks for the review.

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

 #include "../performance/MoveConstructorInitCheck.h"
 #include "../readability/UppercaseLiteralSuffixCheck.h"
 #include "CommandProcessorCheck.h"
 #include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
 #include "PostfixOperatorCheck.h"
 #include "ProperlySeededRandomGeneratorCheck.h"
+#include "PutenvWithAutoCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // FIO
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
     // ERR
     CheckFactories.registerCheck<StrToNumCheck>("cert-err34-c");
     // MSC
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc32-c");
+    // POS
+    CheckFactories.registerCheck<PutenvWithAutoCheck>("cert-pos34-c");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
     Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU";
     Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "0";
     return Options;

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

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

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

+//===--- PutenvWithAutoCheck.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_PUTENV_WITH_AUTO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_PUTENV_WITH_AUTO_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Finds calls of putenv function with automatic variable as the argument.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-pos34-c.html
+class PutenvWithAutoCheck : public ClangTidyCheck {
+public:
+  PutenvWithAutoCheck(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_PUTENV_WITH_AUTO_H

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

+//===--- PutenvWithAutoCheck.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 "PutenvWithAutoCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+void PutenvWithAutoCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(
+          callee(functionDecl(hasName("::putenv"))),
+          hasArgument(
+              0,
+              declRefExpr(hasDeclaration(varDecl(
+                  hasAutomaticStorageDuration(),
+                  unless(hasDescendant(callExpr(callee(functionDecl(hasAnyName(
+                      "::alloc", "::malloc", "::realloc", "::calloc")))))))))))
+          .bind("func"),
+      this);
+}
+
+void PutenvWithAutoCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *PutenvCall = Result.Nodes.getNodeAs<CallExpr>("func");
+
+  diag(PutenvCall->getBeginLoc(),
+       "'putenv' function should not be called with auto variables");
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New :doc:`bugprone-unhandled-self-assignment
   <clang-tidy/checks/bugprone-unhandled-self-assignment>` check.
 
   Finds user-defined copy assignment operators which do not protect the code
   against self-assignment either by checking self-assignment explicitly or
   using the copy-and-swap or the copy-and-move method.
 
+- New :doc:`cert-pos34-c
+  <clang-tidy/checks/cert-pos34-c>` check.
+
+  Finds calls of ``putenv`` function with automatic variable as the argument.
+
 - New :doc:`fuchsia-default-arguments-calls
   <clang-tidy/checks/fuchsia-default-arguments-calls>` check.
 
   Warns if a function or method is called with default arguments.
   This was previously done by `fuchsia-default-arguments check`, which has been
   removed.
 
 - New :doc:`fuchsia-default-arguments-declarations

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

+.. title:: clang-tidy - cert-pos34-c
+
+cert-pos34-c
+=====================
+
+Finds calls of ``putenv`` function with automatic variable as the argument.
+
+.. code-block:: c
+
+  #include <stdlib.h>
+
+  int func(const char *var) {
+    char env[1024];
+    int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+    if (retval < 0 || (size_t)retval >= sizeof(env)) {
+        /* Handle error */
+    }
+ 
+    return putenv(env); // putenv function should not be called with auto variables
+  }
+
+
+This check corresponds to the CERT Standard rule 
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the argument.
+<https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument>`_.

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

.. toctree::
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc32-c (redirects to cert-msc51-cpp) <cert-msc32-c>
    cert-msc50-cpp
    cert-msc51-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
+   cert-pos34-c
    cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) <cppcoreguidelines-avoid-c-arrays>
    cppcoreguidelines-avoid-goto
    cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers>
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
    cppcoreguidelines-explicit-virtual-functions (redirects to modernize-use-override) <cppcoreguidelines-explicit-virtual-functions>
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-macro-usage
    cppcoreguidelines-narrowing-conversions

clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp

+// RUN: %check_clang_tidy %s cert-pos34-c %t
+#include <stdlib.h>
+#include <string.h>
+
+namespace test_auto_var_used_bad {
+
+int func1(const char *var) {
+  char env[1024];
+  /*
+  Do Something
+  */
+  return putenv(env);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+int func2(char *a) {
+  return putenv(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+void func3(char *a) {
+  putenv(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int func4(const char *var) {
+  static char env[1024];
+  /*
+  Do Something
+  */
+  return putenv(env); // no-warning: env is static.
+}
+
+// example from cert
+int func5(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+    return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+    free(env);
+    return -1;
+  }
+  if (oldenv != NULL) {
+    free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+extern char *testExt;
+int func6() {
+  return putenv(testExt); // no-warning: extern storage class.
+}
+
+} // namespace test_auto_var_used_good