[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

1 ↗

(On Diff #118493)

We need to have the license preamble here.

3 ↗

(On Diff #118493)

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.

17 ↗

(On Diff #118493)

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'?

22 ↗

(On Diff #118493)

Does the constructor need to take an AnalyzerOptions?

53 ↗

(On Diff #118493)

Should we just early return if location is not valid?

70 ↗

(On Diff #118493)

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.

71 ↗

(On Diff #118493)

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

74 ↗

(On Diff #118493)

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());

93 ↗

(On Diff #118493)

Do you really want a bit-wise or here?

113 ↗

(On Diff #118493)

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

41 ↗

(On Diff #118493)

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

In this scenario v can be reassigned, right?

45 ↗

(On Diff #118493)

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

55 ↗

(On Diff #118493)

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.

cfe/trunk/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 NonnullStringConstantsChecker: 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">;
 

cfe/trunk/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
+  NonnullStringConstantsChecker.cpp
   NullabilityChecker.cpp
   NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp

+//==- NonnullStringConstantsChecker.cpp ---------------------------*- C++ -*--//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This checker adds an assumption that constant string-like globals are
+//  non-null, as otherwise they generally do not convey any useful information.
+//  The assumption is useful, as many framework use such global const strings,
+//  and the analyzer might not be able to infer the global value if the
+//  definition is in a separate translation unit.
+//  The following types (and their typedef aliases) are considered string-like:
+//   - `char* const`
+//   - `const CFStringRef` from CoreFoundation
+//   - `NSString* const` from Foundation
+//
+//===----------------------------------------------------------------------===//
+
+#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 NonnullStringConstantsChecker : public Checker<check::Location> {
+  mutable IdentifierInfo *NSStringII = nullptr;
+  mutable IdentifierInfo *CFStringRefII = nullptr;
+
+public:
+  NonnullStringConstantsChecker() {}
+
+  void checkLocation(SVal l, bool isLoad, const Stmt *S,
+                     CheckerContext &C) const;
+
+private:
+  void initIdentifierInfo(ASTContext &Ctx) const;
+
+  bool isGlobalConstString(SVal V) const;
+
+  bool isStringlike(QualType Ty) const;
+};
+
+} // namespace
+
+/// Lazily initialize cache for required identifier informations.
+void NonnullStringConstantsChecker::initIdentifierInfo(ASTContext &Ctx) const {
+  if (NSStringII)
+    return;
+
+  NSStringII = &Ctx.Idents.get("NSString");
+  CFStringRefII = &Ctx.Idents.get("CFStringRef");
+}
+
+/// Add an assumption that const string-like globals are non-null.
+void NonnullStringConstantsChecker::checkLocation(SVal location, bool isLoad,
+                                                 const Stmt *S,
+                                                 CheckerContext &C) const {
+  initIdentifierInfo(C.getASTContext());
+  if (!isLoad || !location.isValid())
+    return;
+
+  ProgramStateRef State = C.getState();
+  SVal 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);
+    }
+  }
+}
+
+/// \param V loaded lvalue.
+/// \return whether {@code val} is a string-like const global.
+bool NonnullStringConstantsChecker::isGlobalConstString(SVal V) const {
+  Optional<loc::MemRegionVal> RegionVal = V.getAs<loc::MemRegionVal>();
+  if (!RegionVal)
+    return false;
+  auto *Region = dyn_cast<VarRegion>(RegionVal->getAsRegion());
+  if (!Region)
+    return false;
+  const VarDecl *Decl = Region->getDecl();
+
+  if (!Decl->hasGlobalStorage())
+    return false;
+
+  QualType Ty = Decl->getType();
+  bool HasConst = Ty.isConstQualified();
+  if (isStringlike(Ty) && HasConst)
+    return true;
+
+  // Look through the typedefs.
+  while (auto *T = dyn_cast<TypedefType>(Ty)) {
+    Ty = T->getDecl()->getUnderlyingType();
+
+    // It is sufficient for any intermediate typedef
+    // to be classified const.
+    HasConst = HasConst || Ty.isConstQualified();
+    if (isStringlike(Ty) && HasConst)
+      return true;
+  }
+  return false;
+}
+
+/// \return whether {@code type} is a string-like type.
+bool NonnullStringConstantsChecker::isStringlike(QualType Ty) const {
+
+  if (Ty->isPointerType() && Ty->getPointeeType()->isCharType())
+    return true;
+
+  if (auto *T = dyn_cast<ObjCObjectPointerType>(Ty)) {
+    return T->getInterfaceDecl()->getIdentifier() == NSStringII;
+  } else if (auto *T = dyn_cast<TypedefType>(Ty)) {
+    return T->getDecl()->getIdentifier() == CFStringRefII;
+  }
+  return false;
+}
+
+void ento::registerNonnullStringConstantsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<NonnullStringConstantsChecker>();
+}

cfe/trunk/test/Analysis/nonnull-string-constants.mm

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// Nullability of const string-like globals, testing
+// NonnullStringConstantsChecker.
+
+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}}
+}
+
+// char* const is also assumed to be non-null.
+extern const char *const ConstCharStarConst;
+void constCharStarCheckGlobal() {
+  clang_analyzer_eval(ConstCharStarConst); // expected-warning{{TRUE}}
+}
+
+// Pointer value can be mutable.
+extern char *const CharStarConst;
+void charStarCheckGlobal() {
+  clang_analyzer_eval(CharStarConst); // expected-warning{{TRUE}}
+}
+
+// But the pointer itself should be immutable.
+extern char *CharStar;
+void charStartCheckMutableGlobal() {
+  clang_analyzer_eval(CharStar); // expected-warning{{UNKNOWN}}
+}
+
+// Type definitions should also work across typedefs, for pointers:
+typedef char *const 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}}
+}
+
+// And for NSString *.
+typedef NSString *const nstr1;
+typedef nstr1 nstr2;
+extern nstr2 nglobalStr2;
+void testNestedTypedefsForNSString() {
+  clang_analyzer_eval(nglobalStr2); // expected-warning{{TRUE}}
+}