[Analyzer] Assume const string-like globals are non-null

[dcoughlin]

Looks like a great start!

There are a bunch of minor nits inline.

The one big thing is that I think your handling of 'const char *' in typeIsConstString() isn't quite right. 'const char *' means that the pointed-to characters can't be modified but does allow modification of the variable holding the pointer. I don't think we want to treat such variables as holding non-null pointers, since anyone could assign them to NULL. On the other hand, we do want to treat variables of type 'char * const' as holding non-null pointers since the variable can't be reassigned and (presumably) it was initialized to a non-null value. In this second case the characters could potentially be modified, but that doesn't change whether the value of the pointer itself.

lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp

2

We need to have the license preamble here.

4

It would be good to include a motivation for why this is the right thing to do and even an example of the declarations this will trigger on.

18

Since this applies to more than just Objective-C, I think it would be better to use 'null' instead of 'nil' in the name of the checker. Or even remove the 'Nonnil' prefix. What about 'GlobalStringConstantsChecker'?

23

Does the constructor need to take an AnalyzerOptions?

54

Should we just early return if location is not valid?

71

Can you add doxygen-style comments to the implementations of these methods? I'd like to break with tradition and have comments for all new code.

72

Note that we use capital letters for variables and parameters in Clang/LLVM.

75

The convention Clang uses with dyn_cast and friends to to use 'auto *' in the variable declaration in order to not repeat the type name:

auto *Region = dyn_cast<VarRegion>(RegionVal->getAsRegion());

94

Do you really want a bit-wise or here?

114

This will leave II as uninitialized if your if/else if is not exhaustive. Are you sure that it is?

test/Analysis/nonnil-string-constants.mm

42

What is the rationale for treating const char *v as non null?

In this scenario v can be reassigned, right?

46

I'd also like to see a test for treating char * const v; as non null.

56

typedef const char *str;
extern str globalStr;

allows the globalStr variable to be written to.

Did you mean:

typedef char * const str;
extern str globalStr;

[george.karpenkov]

Adhering to comments.

[george.karpenkov]

Marking requests as "done".

[george.karpenkov]

Typo fix.

[dcoughlin]

Looks good to me. Please fix the additional nits mentioned inline and commit! Also, make sure to do a pass to update the capitalization of variables throughout the file to match the LLVM coding standards. https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp

1 ↗

(On Diff #118522)

The file name and description here needs to be updated for this checker.

10 ↗

(On Diff #118522)

I don't think the comment in the first line here is needed.

22 ↗

(On Diff #118522)

We generally don't do this kind of cross-reference in comments since they tend to get stale fast when things get moved around. There is no tool support to keep them in sync.

26 ↗

(On Diff #118522)

Can this comment go?

53 ↗

(On Diff #118522)

We usually put the oxygen comments on checkers on the implementation and not the interface since they aren't API and people reading the code generally look at the implementations. Can you move them to the implementations?

114 ↗

(On Diff #118522)

To match the coding standards this should be auto * as well.

131 ↗

(On Diff #118522)

Similar comment about auto *

133 ↗

(On Diff #118522)

Similar comment about auto *.

test/Analysis/nonnull-string-constants.mm

5 ↗

(On Diff #118522)

We generally don't put file paths like this in comments since they tend to go stale. It is fine to to say that these are tests for the NonnullStringConstantsChecker though.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp

22 ↗

(On Diff #118522)

:(
I guess I'm too spoiled by Javadoc.

53 ↗

(On Diff #118522)

OK. I assume there's no good universal solution here.

include/clang/StaticAnalyzer/Checkers/Checkers.td

 def StackAddrEscapeChecker : Checker<"StackAddressEscape">,
   HelpText<"Check that addresses to stack memory do not escape the function">,
   DescFile<"StackAddrEscapeChecker.cpp">;
 
 def DynamicTypePropagation : Checker<"DynamicTypePropagation">,
   HelpText<"Generate dynamic type information">,
   DescFile<"DynamicTypePropagation.cpp">;
 
+def NonnilStringConstantsChecker: Checker<"NonnilStringConstants">,
+  HelpText<"Assume that const string-like globals are non-null">,
+  DescFile<"NonilStringConstantsChecker.cpp">;
+
 } // end "core"
 
 let ParentPackage = CoreAlpha in {
 
 def BoolAssignmentChecker : Checker<"BoolAssignment">,
   HelpText<"Warn about assigning non-{0,1} values to Boolean variables">,
   DescFile<"BoolAssignmentChecker.cpp">;
 

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   MisusedMovedObjectChecker.cpp
   MPI-Checker/MPIBugReporter.cpp
   MPI-Checker/MPIChecker.cpp
   MPI-Checker/MPIFunctionClassifier.cpp
   NSAutoreleasePoolChecker.cpp
   NSErrorChecker.cpp
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
+  NonnilStringConstantsChecker.cpp
   NullabilityChecker.cpp
   NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp

lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp

+
+//
+// This checker ensures that const string globals are assumed to be non-null.
+//
+#include "ClangSACheckers.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/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class NonnilStringConstantsChecker : public Checker<check::Location> {
+  mutable IdentifierInfo *NSStringII;
+  mutable IdentifierInfo *CFStringRefII;
+
+public:
+  NonnilStringConstantsChecker(AnalyzerOptions &AO) {}
+
+  void checkLocation(SVal l, bool isLoad, const Stmt *S,
+                     CheckerContext &C) const;
+
+private:
+  /// Lazily initialize cache for required identifier informations.
+  void initIdentifierInfo(ASTContext &Ctx) const;
+  bool typeIsConstString(QualType type, bool isConstQualified) const;
+  bool isGlobalConstString(SVal val) const;
+};
+
+} // namespace
+
+void NonnilStringConstantsChecker::checkLocation(SVal location, bool isLoad,
+                                                 const Stmt *S,
+                                                 CheckerContext &C) const {
+  initIdentifierInfo(C.getASTContext());
+  if (!isLoad)
+    return;
+
+  ProgramStateRef State = C.getState();
+  SVal V = UnknownVal();
+  if (location.isValid()) {
+    V = State->getSVal(location.castAs<Loc>());
+  }
+
+  if (isGlobalConstString(location)) {
+    Optional<DefinedOrUnknownSVal> Constr = V.getAs<DefinedOrUnknownSVal>();
+
+    if (Constr) {
+
+      // Assume that the variable is non-null.
+      ProgramStateRef OutputState = State->assume(*Constr, true);
+      C.addTransition(OutputState);
+    }
+  }
+}
+
+void NonnilStringConstantsChecker::initIdentifierInfo(ASTContext &Ctx) const {
+  if (NSStringII)
+    return;
+
+  NSStringII = &Ctx.Idents.get("NSString");
+  CFStringRefII = &Ctx.Idents.get("CFStringRef");
+}
+
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+  Optional<loc::MemRegionVal> regionVal = val.getAs<loc::MemRegionVal>();
+  if (!regionVal)
+    return false;
+  const VarRegion *region = dyn_cast<VarRegion>(regionVal->getAsRegion());
+  if (!region)
+    return false;
+  const VarDecl *decl = region->getDecl();
+
+  if (!decl->hasGlobalStorage())
+    return false;
+
+  QualType type = decl->getType();
+  bool hasOuterConst = type.isConstQualified();
+  if (typeIsConstString(type, hasOuterConst))
+    return true;
+
+  // Look through the typedefs.
+  while (const TypedefType *T = dyn_cast<TypedefType>(type)) {
+    type = T->getDecl()->getUnderlyingType();
+
+    // It is sufficient for any intermediate typedef
+    // to be classified const.
+    hasOuterConst |= type.isConstQualified();
+    if (typeIsConstString(type, hasOuterConst))
+      return true;
+  }
+  return false;
+}
+
+bool NonnilStringConstantsChecker::typeIsConstString(
+    QualType type, bool isConstQualified) const {
+
+  // Match `const char*`.
+  if (type->isPointerType() && type->getPointeeType()->isCharType() &&
+      type->getPointeeType().isConstQualified())
+    return true;
+
+  IdentifierInfo *II;
+  if (const ObjCObjectPointerType *T = dyn_cast<ObjCObjectPointerType>(type)) {
+    II = T->getInterfaceDecl()->getIdentifier();
+  } else if (const TypedefType *T = dyn_cast<TypedefType>(type)) {
+    II = T->getDecl()->getIdentifier();
+  }
+
+  // Match `NSString* const` or `const CFStringRef`.
+  return isConstQualified && (II == NSStringII || II == CFStringRefII);
+}
+
+void ento::registerNonnilStringConstantsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<NonnilStringConstantsChecker>(Mgr.getAnalyzerOptions());
+}

test/Analysis/nonnil-string-constants.mm

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// Nullability of const string-like globals.
+void clang_analyzer_eval(bool);
+
+@class NSString;
+typedef const struct __CFString *CFStringRef;
+
+// Global NSString* is non-null.
+extern NSString *const StringConstGlobal;
+void stringConstGlobal() {
+  clang_analyzer_eval(StringConstGlobal); // expected-warning{{TRUE}}
+}
+
+// The logic does not apply to local variables though.
+extern NSString *stringGetter();
+void stringConstLocal() {
+  NSString *const local = stringGetter();
+  clang_analyzer_eval(local); // expected-warning{{UNKNOWN}}
+}
+
+// Global const CFStringRef's are also assumed to be non-null.
+extern const CFStringRef CFStringConstGlobal;
+void cfStringCheckGlobal() {
+  clang_analyzer_eval(CFStringConstGlobal); // expected-warning{{TRUE}}
+}
+
+// But only "const" ones.
+extern CFStringRef CFStringNonConstGlobal;
+void cfStringCheckMutableGlobal() {
+  clang_analyzer_eval(CFStringNonConstGlobal); // expected-warning{{UNKNOWN}}
+}
+
+// Const char* is also assumed to be non-null.
+extern const char *const ConstCharStarConst;
+void constCharStarCheckGlobal() {
+  clang_analyzer_eval(ConstCharStarConst); // expected-warning{{TRUE}}
+}
+
+// For char* we do not require a pointer itself to be immutable.
+extern const char *CharStarConst;
+void charStarCheckGlobal() {
+  clang_analyzer_eval(CharStarConst); // expected-warning{{TRUE}}
+}
+
+// But the pointed data should be.
+extern char *CharStar;
+void charStartCheckMutableGlobal() {
+  clang_analyzer_eval(CharStar); // expected-warning{{UNKNOWN}}
+}
+
+// Type definitions should also work across typedefs, for pointers:
+typedef const char *str;
+extern str globalStr;
+void charStarCheckTypedef() {
+  clang_analyzer_eval(globalStr); // expected-warning{{TRUE}}
+}
+
+// And for types.
+typedef NSString *const NStr;
+extern NStr globalNSString;
+void NSStringCheckTypedef() {
+  clang_analyzer_eval(globalNSString); // expected-warning{{TRUE}}
+}
+
+// Note that constness could be either inside
+// the var declaration, or in a typedef.
+typedef NSString *NStr2;
+extern const NStr2 globalNSString2;
+void NSStringCheckConstTypedef() {
+  clang_analyzer_eval(globalNSString2); // expected-warning{{TRUE}}
+}
+
+// Nested typedefs should work as well.
+typedef const CFStringRef str1;
+typedef str1 str2;
+extern str2 globalStr2;
+void testNestedTypedefs() {
+  clang_analyzer_eval(globalStr2); // expected-warning{{TRUE}}
+}