[PATCH] Github Issue: Create a check that warns about using %p printf specifier #43453

[giannicrivello]

Summary

Patched llvm-project/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp to account for the user passing a nullptr to the implementation dependent function printf(). Invoked with bin/scan-build -enable-checker optin.portability bin/clang -g test.cpp where the contents of test.cpp is:

c++
#include <stdio.h>
#include <stdlib.h>

int main() {
    int *p = nullptr;
    printf("%p", p);
}

now produces the warning:

test.cpp:6:5: warning: Passing a null pointer to printf() is implementation dependant. Portability warning [optin.portability.UnixAPI]
    printf("%p", p);
    ^~~~~~~~~~~~~~~
1 warning generated.

[steakhal]

I appreciate that you are interested in tackling this. I do think it's a low-hanging fruit, so it's a good choice!

However, there are a couple of things we need to improve before we could proceed:

1. Add tests demonstrating all possible aspects of your change.
2. Generally, one should check preconditions in the Pre checker callbacks, and apply postconditions (effects) in Post checker callbacks.
3. We should have a specific BugType pointer for the specific bug you hunt - in other words you should not reuse BT_mallocZero for your checker.
4. One should be able to disable your rule in case something goes wild. So, your checker should be registered at the end of the file by applying the REGISTER_CHECKER() macro.
5. One should update the documentation and also mention the new check in the release docs to give visibility.
6. For matching if a function is interesting or not, you should use CallDescriptions instead of looking at the IdentifierInfo directly - unless there are other cases in the scope where we don't use this facility. However, for those case we should consider updating those to use CallDescriptionSet|Map.
7. I can see that you directly use the isZeroConstant() on the value of the pointer. This might not work for all cases, e.g. when the pointer is symbolic. In that case, we might be able to prove that the pointer should be null, because the path-constraints we collected so far imply that.

Thanks for improving the detection!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 // Portability checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = PortabilityOptIn in {
 
 def UnixAPIPortabilityChecker : Checker<"UnixAPI">,
   HelpText<"Finds implementation-defined behavior in UNIX/Posix functions">,
   Documentation<NotDocumented>;
   
 } // end optin.portability
 
 //===----------------------------------------------------------------------===//
 // NonDeterminism checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = NonDeterminismAlpha in {
 

clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp

 //= UnixAPIChecker.h - Checks preconditions for various Unix APIs --*- 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This defines UnixAPIChecker, which is an assortment of checks on calls
 // to various, widely used UNIX/Posix functions.
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
public:
 
   void ReportOpenBug(CheckerContext &C,
                      ProgramStateRef State,
                      const char *Msg,
                      SourceRange SR) const;
 
 };
 
-class UnixAPIPortabilityChecker : public Checker< check::PreStmt<CallExpr> > {
+class UnixAPIPortabilityChecker
+    : public Checker<check::PreStmt<CallExpr>, check::PostCall> {
 public:
   void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
 
 private:
   mutable std::unique_ptr<BugType> BT_mallocZero;
 
   void CheckCallocZero(CheckerContext &C, const CallExpr *CE) const;
   void CheckMallocZero(CheckerContext &C, const CallExpr *CE) const;
   void CheckReallocZero(CheckerContext &C, const CallExpr *CE) const;
   void CheckReallocfZero(CheckerContext &C, const CallExpr *CE) const;
void UnixAPIPortabilityChecker::checkPreStmt(const CallExpr *CE,
 
   else if (FName == "__builtin_alloca_with_align")
     CheckAllocaWithAlignZero(C, CE);
 
   else if (FName == "valloc")
     CheckVallocZero(C, CE);
 }
 
+void UnixAPIPortabilityChecker::checkPostCall(const CallEvent &Call,
+                                              CheckerContext &C) const {
+
+  auto State = C.getState();
+
+  const IdentifierInfo *II = Call.getCalleeIdentifier();
+  if (!II)
+    return;
+  if (!II->isStr("printf"))
+    return;
+
+  if (!BT_mallocZero)
+    BT_mallocZero.reset(new BugType(this, "Call to printf", "Example checker"));
+
+  for (unsigned int i = 0; i < Call.getNumArgs(); i++) {
+    const auto *Arg = Call.getArgExpr(i);
+    if (!Arg)
+      return;
+    const auto *LC = C.getLocationContext();
+    auto Val = State->getSVal(Arg, LC);
+    if (Val.isZeroConstant()) {
+      ExplodedNode *N = C.generateErrorNode();
+
+      // Further better the diagnostic message by adding a bug report visitor
+      auto Report = std::make_unique<PathSensitiveBugReport>(
+          *BT_mallocZero,
+          "Passing a null pointer to printf() is implementation dependant. "
+          "Portability warning.",
+          N);
+      Report->addRange(Arg->getSourceRange());
+      C.emitReport(std::move(Report));
+    }
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // Registration.
 //===----------------------------------------------------------------------===//
 
 #define REGISTER_CHECKER(CHECKERNAME)                                          \
   void ento::register##CHECKERNAME(CheckerManager &mgr) {                      \
     mgr.registerChecker<CHECKERNAME>();                                        \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##CHECKERNAME(const CheckerManager &mgr) {          \
     return true;                                                               \
   }
 
 REGISTER_CHECKER(UnixAPIMisuseChecker)
 REGISTER_CHECKER(UnixAPIPortabilityChecker)

flang/docs/Extensions.md

* A digit count of d=0 is accepted in Ew.0, Dw.0, and Gw.0 output
   editing if no nonzero scale factor (kP) is in effect.
 * The name `IMAG` is accepted as an alias for the generic intrinsic
   function `AIMAG`.
 * The legacy extension intrinsic functions `IZEXT` and `JZEXT`
   are supported; `ZEXT` has different behavior with various older
   compilers, so it is not supported.
 * f18 doesn't impose a limit on the number of continuation lines
   allowed for a single statement.
+* When a type-bound procedure declaration statement has neither interface
+  nor attributes, the "::" before the bindings is optional, even
+  if a binding has renaming with "=> proc".
+  The colons are not necessary for an unambiguous parse, C768
+  notwithstanding.
 
 ### Extensions supported when enabled by options
 
 * C-style backslash escape sequences in quoted CHARACTER literals
   (but not Hollerith) [-fbackslash]
 * Logical abbreviations `.T.`, `.F.`, `.N.`, `.A.`, `.O.`, and `.X.`
   [-flogical-abbreviations]
 * `.XOR.` as a synonym for `.NEQV.` [-fxor-operator]

flang/lib/Parser/Fortran-parsers.cpp

recovery(
             construct<TypeBoundProcBinding>(Parser<TypeBoundGenericStmt>{}),
             construct<TypeBoundProcBinding>(Parser<FinalProcedureStmt>{})),
         construct<TypeBoundProcBinding>(
             !"END"_tok >> SkipTo<'\n'>{} >> construct<ErrorRecovery>())))
 
 // R749 type-bound-procedure-stmt ->
 //        PROCEDURE [[, bind-attr-list] ::] type-bound-proc-decl-list |
 //        PROCEDURE ( interface-name ) , bind-attr-list :: binding-name-list
+// The "::" is required by the standard (C768) in the first production if
+// any type-bound-proc-decl has a "=>', but it's not strictly necessary to
+// avoid a bad parse.
 TYPE_CONTEXT_PARSER("type bound PROCEDURE statement"_en_US,
     "PROCEDURE" >>
         (construct<TypeBoundProcedureStmt>(
              construct<TypeBoundProcedureStmt::WithInterface>(
                  parenthesized(name),
                  localRecovery("expected list of binding attributes"_err_en_US,
                      "," >> nonemptyList(Parser<BindAttr>{}), ok),
                  localRecovery("expected list of binding names"_err_en_US,
                      "::" >> listOfNames, SkipTo<'\n'>{}))) ||
+            construct<TypeBoundProcedureStmt>(construct<
+                TypeBoundProcedureStmt::WithoutInterface>(
+                pure<std::list<BindAttr>>(),
+                nonemptyList(
+                    "expected type bound procedure declarations"_err_en_US,
+                    construct<TypeBoundProcDecl>(name,
+                        maybe(extension<LanguageFeature::MissingColons>(
+                            "type-bound procedure statement should have '::' if it has '=>'"_port_en_US,
+                            "=>" >> name)))))) ||
             construct<TypeBoundProcedureStmt>(
                 construct<TypeBoundProcedureStmt::WithoutInterface>(
                     optionalListBeforeColons(Parser<BindAttr>{}),
                     nonemptyList(
                         "expected type bound procedure declarations"_err_en_US,
                         Parser<TypeBoundProcDecl>{})))))
 
 // R750 type-bound-proc-decl -> binding-name [=> procedure-name]

flang/test/Parser/missing-colons.f90

+! RUN: %flang_fc1 -fsyntax-only -pedantic %s 2>&1 | FileCheck %s
+module m
+  type t
+   contains
+!CHECK: portability: type-bound procedure statement should have '::' if it has '=>'
+    procedure p => sub
+  end type
+ contains
+  subroutine sub(x)
+    class(t), intent(in) :: x
+  end subroutine
+end module
+