[analyzer] Add option for AddrSpace in core.NullDereference check

[vabridgers]

Summary

This change adds an option to detect all null dereferences for
non-default address spaces, except for address spaces 256, 257 and 258.
Those address spaces are special since null dereferences are not errors.

All address spaces can be considered (except for 256, 257, and 258) by
using -analyzer-config
core.NullDereference:DetectAllNullDereferences=true. This option is
false by default, retaining the original behavior.

A LIT test was enhanced to cover this case, and the rst documentation
was updated to describe this behavior.

[steakhal]

Couls you please provide some context about why did we have this suppression in the first place?

Other than that, I'm on board with this, but we should really investigate the historical reasons why we had this.

If it turns out to be a valid usecase, but a rather special one we could still have an analyzer config option making this change optional.

@NoQ do you have any background on this?

[vabridgers]

@steakhal, good question. Digging into the change archaeology, I see this change was made by Anna Zaks in Jan 2016 - see commit header below.

There's no reference to a review with comments, and I don't know how to find those if they exist, perhaps @NoQ could help with that?

In any case, judging by the changes I had to make to the test cases, there were no regressions in the current test set, and at least our build tests ran ok - seems like this is no longer necessary. But I understand the need to approach this carefully.

Thanks

commit c9f16fe48c40e7683d0029b65090d0007414d7af
Author: Anna Zaks <[e-mail]>
Date:   Wed Jan 6 00:32:49 2016 +0000

    [analyzer] Don't report null dereferences on address_space annotated memory
    
    llvm-svn: 256885

[NoQ]

I looked up the history. I believe this refers to https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments:

Annotating a pointer with address space #256 causes it to be code generated relative to the X86 GS segment register, address space #257 causes it to be relative to the X86 FS segment, and address space #258 causes it to be relative to the X86 SS segment. Note that this is a very very low-level feature that should only be used if you know what you’re doing (for example in an OS kernel).

So basically ((void *__attribute__((address_space(256))) *)0) is a valid pointer that can be safely dereferenced.

I'm not aware of other situations of this kind. Probably this address space range can be hardcoded, and/or a run-time checker option can be provided to enable/disable this behavior.

[steakhal]

In D122841#3431617, @NoQ wrote:

I looked up the history. I believe this refers to https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments:

Annotating a pointer with address space #256 causes it to be code generated relative to the X86 GS segment register, address space #257 causes it to be relative to the X86 FS segment, and address space #258 causes it to be relative to the X86 SS segment. Note that this is a very very low-level feature that should only be used if you know what you’re doing (for example in an OS kernel).

So basically ((void *__attribute__((address_space(256))) *)0) is a valid pointer that can be safely dereferenced.

Well, it seems like, on Windows, the FS register points to the Win32 Thread Environment Block - TEB (wiki). And at offset 0x0, resides the pointer to the Current Structured Exception Handling (SEH) frame, making it perfectly valid to load and dereference FS:[0]. Using FS and GS registers are frequently used for implementing anti-debug tricks in practice, but I think it's fairly safe to say that they are not our primary audience.

I'm not aware of other situations of this kind. Probably this address space range can be hardcoded, and/or a run-time checker option can be provided to enable/disable this behavior.

That being said, I think it might deserve a user-facing analyzer config option.

[vabridgers]

Ahhh, I see - thanks for the references and discussion. I'll craft a patch to comprehend the use case and user-facing option, and post for review. We'll see how it goes :)

[vabridgers]

add option, retain legacy behavior per comments. update rst

[vabridgers]

clean up a few debug edits

[steakhal]

clang/docs/analyzer/checkers.rst

69–74 ↗

(On Diff #421521)

IMO this should be in a separate paragraph. It's such a niche use-case.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

114

Shouldn't we check for this target?
Add a test if we have x86 and a non-x86 target.

120–128

Even though DetectAllNullDereferences=true, address_space(256) would be suppressed, which seems to be a contradiction to me.

We should either pick a different name or implement a different semantic.
I would vote for both, personally.

Then name should encode that it's purely about address-space attributes.
The implementation should check if we have attributes or not, and bail out in the generic case. I would do something like this:

QualType Ty = E->getType();
if (!Ty.hasAddressSpace())
  return false;
if (SuppressAddressSpaces)
  return true;
// On X86 addr spaces ...., thus ignored.
return target == x86 && addressspace in {256,257,258};

302

dead-code?

[vabridgers]

I'll address, thanks for the comments.

clang/docs/analyzer/checkers.rst

69–74 ↗

(On Diff #421521)

I'll address.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

114

I wasn't sure it was needed, but easy enough to add I think. I'll address both.

120–128

I'll clean up the semantics. Thanks.

[vabridgers]

update per @steakhal's comments

[vabridgers]

I believe all points have been addressed. Thanks for the comments.

[steakhal]

No major issues left.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

53

If I'm correct, we tend to use the 'DefaultBool' instead of plain old bools for checker options. Could you please check if this is the case and consolidate this?

302

This still looks dead.

clang/test/Analysis/cast-value-notes.cpp

19

Nit: the default triple is the host/native triple, but you did actually pin it to x86. Hence it might not be the default on other platforms.

[vabridgers]

Thanks for the the comments. I'll make the changes and resubmit. Please let me know your preferences for the test case (default vs pinned x86 or same).

Best!

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

53

I'll address. FYI, looks like I chose the red pill when I found an example to go by :/ Looks like there's opportunity for some cleanup NFC type of patches. I'll pick these up in a separate patch.

120 namespace {
121 class DeadStoresChecker : public Checker<check::ASTCodeBody> {
122 public:
123   bool ShowFixIts = false;
124   bool WarnForDeadNestedAssignments = true;
125 
126   void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
127                         BugReporter &BR) const;
128 };

302

arg, missed that. I'll clean this up.

clang/test/Analysis/cast-value-notes.cpp

19

Yeah, the result is the same (as far as what the test code would look like). Not sure what your preferences are here.

Would you prefer to see separate cases (default and pinned x86), or perhaps the macro names renamed to something that means both (instead of implying default)?

[vabridgers]

update per @steakhal's latest comments

[vabridgers]

The two small issues are addressed, I think the only outstanding issue is the question about the test case. Please let me know a preference and I'll implement accordingly. Best!

[vabridgers]

Update test cases, address comments

[vabridgers]

I believe all comments have been addressed. After discussion with @steakhal, we're leaning towards removing the DefaultBool class and refactoring in favor of the language native bool type - mainly because DefaultBool doesn't add anything. Minimal yet effective is usually better.

[vabridgers]

add back amdgcn cases

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

public:
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
 
   static void AddDerefSource(raw_ostream &os,
                              SmallVectorImpl<SourceRange> &Ranges,
                              const Expr *Ex, const ProgramState *state,
                              const LocationContext *LCtx,
                              bool loadedFrom = false);
 };
 } // end anonymous namespace
 
 void
 DereferenceChecker::AddDerefSource(raw_ostream &os,
                                    SmallVectorImpl<SourceRange> &Ranges,
                                    const Expr *Ex,
                                    const ProgramState *state,
                                    const LocationContext *LCtx,
                                    bool loadedFrom) {
if (IsBind) {
     const Expr *Init;
     std::tie(VD, Init) = parseAssignment(S);
     if (VD && Init)
       E = Init;
   }
   return E;
 }
 
-static bool suppressReport(const Expr *E) {
-  // Do not report dereferences on memory in non-default address spaces.
-  return E->getType().hasAddressSpace();
-}
-
 static bool isDeclRefExprToReference(const Expr *E) {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
     return DRE->getDecl()->getType()->isReferenceType();
   return false;
 }
 
 void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State,
                                    const Stmt *S, CheckerContext &C) const {
   const BugType *BT = nullptr;
   llvm::StringRef DerefStr1;
   llvm::StringRef DerefStr2;
   switch (K) {
   case DerefKind::NullPointer:
     BT = &BT_Null;
     DerefStr1 = " results in a null pointer dereference";
     DerefStr2 = " results in a dereference of a null pointer";
     break;
   case DerefKind::UndefinedPointerValue:
     BT = &BT_Undef;
     DerefStr1 = " results in an undefined pointer dereference";
     DerefStr2 = " results in a dereference of an undefined pointer value";
     break;
   };
 
   // Generate an error node.
void DereferenceChecker::reportBug(DerefKind K, ProgramStateRef State,
   C.emitReport(std::move(report));
 }
 
 void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
                                        CheckerContext &C) const {
   // Check for dereference of an undefined value.
   if (l.isUndef()) {
     const Expr *DerefExpr = getDereferenceExpr(S);
-    if (!suppressReport(DerefExpr))
-      reportBug(DerefKind::UndefinedPointerValue, C.getState(), DerefExpr, C);
     return;
   }
 
   DefinedOrUnknownSVal location = l.castAs<DefinedOrUnknownSVal>();
 
   // Check for null dereferences.
   if (!location.getAs<Loc>())
     return;
 
   ProgramStateRef state = C.getState();
 
   ProgramStateRef notNullState, nullState;
   std::tie(notNullState, nullState) = state->assume(location);
 
   if (nullState) {
     if (!notNullState) {
       // We know that 'location' can only be null.  This is what
       // we call an "explicit" null dereference.
       const Expr *expr = getDereferenceExpr(S);
-      if (!suppressReport(expr)) {
-        reportBug(DerefKind::NullPointer, nullState, expr, C);
-        return;
-      }
-    }
 
     // Otherwise, we have the case where the location could either be
     // null or not-null.  Record the error node as an "implicit" null
     // dereference.
     if (ExplodedNode *N = C.generateSink(nullState, C.getPredecessor())) {
       ImplicitNullDerefEvent event = {l, isLoad, N, &C.getBugReporter(),
                                       /*IsDirectDereference=*/true};
       dispatchEvent(event);
void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
   ProgramStateRef State = C.getState();
 
   ProgramStateRef StNonNull, StNull;
   std::tie(StNonNull, StNull) = State->assume(V.castAs<DefinedOrUnknownSVal>());
 
   if (StNull) {
     if (!StNonNull) {
       const Expr *expr = getDereferenceExpr(S, /*IsBind=*/true);
-      if (!suppressReport(expr)) {
-        reportBug(DerefKind::NullPointer, StNull, expr, C);
-        return;
-      }
-    }
 
     // At this point the value could be either null or non-null.
     // Record this as an "implicit" null dereference.
     if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) {
       ImplicitNullDerefEvent event = {V, /*isLoad=*/true, N,
                                       &C.getBugReporter(),
                                       /*IsDirectDereference=*/true};
       dispatchEvent(event);
void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
   //
   // We do need to add a transition because we may have generated a sink for
   // the "implicit" null dereference.
   C.addTransition(State, this);
 }
 
 void ento::registerDereferenceChecker(CheckerManager &mgr) {
   mgr.registerChecker<DereferenceChecker>();
 }
 
 bool ento::shouldRegisterDereferenceChecker(const CheckerManager &mgr) {
   return true;
 }

clang/test/Analysis/cast-value-notes.cpp

-// RUN: %clang_analyze_cc1 -std=c++14 \
-// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
+// R UN: %clang_analyze_cc1 -std=c++14 \
+// R UN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// R UN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
 //
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
 // RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
 
 #include "Inputs/llvm.h"
 
 // The amggcn triple case uses an intentionally different address space.
 // The core.NullDereference checker intentionally ignores checks
 // that use address spaces, so the case is differentiated here.
 //
 // From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
 // select address space 3 (local), since the pointer size is
 // different than Generic.
 #define DEVICE __attribute__((address_space(3)))
 
 namespace clang {
 struct Shape {
   template <typename T>
   const T *castAs() const;
 
   template <typename T>
   const T *getAs() const;
 };
void evalReferences(const Shape &S) {
   clang_analyzer_printState();
   // DEFAULT-CHECK: "dynamic_types": [
   // DEFAULT-CHECK-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
   (void)C;
 }
 #elif defined(AMDGCN_TRIPLE)
 void evalReferences(const Shape &S) {
   const auto &C = dyn_cast<DEVICE Circle>(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
   // AMDGCN-CHECK: "dynamic_types": [
   // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0<const struct clang::Shape & S>}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
   (void)C;
 }
 #else
 #error Target must be specified, and must be pinned
 #endif

clang/test/Analysis/null-deref-ps.c

int foo10595327(int b) {
   if (x == y)
       return *p; // no-warning
   return 0;
 }
 
 #define AS_ATTRIBUTE volatile __attribute__((address_space(256)))
 #define _get_base() ((void * AS_ATTRIBUTE *)0)
 void* test_address_space_array(unsigned long slot) {
-  return _get_base()[slot]; // no-warning
+  return _get_base()[slot]; // expected-warning{{Array access results in a null pointer dereference}}
 }
 void test_address_space_condition(int AS_ATTRIBUTE *cpu_data) {
    if (cpu_data == 0) {
-    *cpu_data = 3; // no-warning
+     *cpu_data = 3; // expected-warning{{Dereference of null pointer}}
   }
 }
 struct X { int member; };
 int test_address_space_member(void) {
   struct X AS_ATTRIBUTE *data = (struct X AS_ATTRIBUTE *)0UL;
   int ret;
-  ret = data->member; // no-warning
+  ret = data->member; // expected-warning{{Access to field 'member' results in a dereference of a null pointer}}
   return ret;
 }

clang/test/Analysis/nullptr.cpp

((X *)returnsNullPtrType())->f(); // expected-warning{{Called C++ object pointer is null}}
                                     // expected-note@-1{{Called C++ object pointer is null}}
 }
 
 #define AS_ATTRIBUTE __attribute__((address_space(256)))
 class AS1 {
 public:
   int x;
   ~AS1() {
-    int AS_ATTRIBUTE *x = 0;
-    *x = 3; // no-warning
+    int AS_ATTRIBUTE *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+    *x = 3;                  // expected-warning{{Dereference of null pointer}}
+                             // expected-note@-1{{Dereference of null pointer}}
   }
 };
 void test_address_space_field_access() {
-  AS1 AS_ATTRIBUTE *pa = 0;
-  pa->x = 0; // no-warning
+  AS1 AS_ATTRIBUTE *pa = 0; // expected-note{{'pa' initialized to a null pointer value}}
+  pa->x = 0;                // expected-warning{{Access to field 'x' results in a dereference of a null pointer (loaded from variable 'pa')}}
+                            // expected-note@-1{{Access to field 'x' results in a dereference of a null pointer (loaded from variable 'pa')}}
 }
 void test_address_space_bind() {
-  AS1 AS_ATTRIBUTE *pa = 0;
-  AS1 AS_ATTRIBUTE &r = *pa;
+  AS1 AS_ATTRIBUTE *pa = 0;  // expected-note{{'pa' initialized to a null pointer value}}
+  AS1 AS_ATTRIBUTE &r = *pa; // expected-warning{{Dereference of null pointer}}
+                             // expected-note@-1{{Dereference of null pointer}}
   r.x = 0; // no-warning
 }