[analyzer] ExprEngine: Escape pointers in bitwise operations

[Charusso]

Summary

After evaluation it would be an Unknown value and tracking would be lost.

[Charusso]

• Make the test fail.

[Charusso]

• Make the test pass.

[Charusso]

These bitwise operations only affects the part of the pointer which we do not analyze. My previous approach to solve this problem caused one false positive, so we should be that strict. Just in case for the future developments, copy-pasted here:

// If we have a bitwise operation (e.g. checking low-bits) of a pointer    
// we would like to keep the pointer to work with it later. Otherwise it  
// would be an Unknown value after evaluation and we would lost tracking.  
if (B->isBitwiseOp()) {                                                    
  bool IsLhsPtr = false, IsRhsPtr = false;                                
  const MemRegion *LeftMR = nullptr, *RightMR = nullptr;                  
                                                                           
  if ((LeftMR = LeftV.getAsRegion()))                                      
    IsLhsPtr = LeftMR->getSymbolicBase();                                  
  if ((RightMR = RightV.getAsRegion()))                                    
    IsRhsPtr = RightMR->getSymbolicBase();                                
                                                                           
  // If only one of the operands is a pointer we should keep it.          
  if (IsLhsPtr ^ IsRhsPtr) {                                              
    SVal KeepV = IsLhsPtr ? LeftV : RightV;                                
    state = state->BindExpr(B, LCtx, KeepV);                              
    Bldr.generateNode(B, *it, state);                                      
    continue;                                                              
  }                                                                        
                                                                           
  // If both of the operands are pointers we would like to keep the one    
  // which dominates. Treat the lower-level pointer as a helper to set up  
  // some low-bits of the higher-level pointer and keep the latter.        
  if (IsLhsPtr && IsRhsPtr) {                                              
    bool IsLhsDominates = false, IsRhsDominates = false;                  
                                                                           
    if (const MemRegion *LeftBase = LeftMR->getBaseRegion())              
      if (const MemSpaceRegion *LeftMS = LeftBase->getMemorySpace())      
        IsLhsDominates = isa<HeapSpaceRegion>(LeftMS);                    
                                                                           
    if (const MemRegion *RightBase = RightMR->getBaseRegion())            
      if (const MemSpaceRegion *RightMS = RightBase->getMemorySpace())    
        IsRhsDominates = isa<HeapSpaceRegion>(RightMS);                    
                                                                           
    if (IsLhsDominates ^ IsRhsDominates) {                                
      SVal KeepDominatorV = IsLhsDominates ? LeftV : RightV;              
      state = state->BindExpr(B, LCtx, KeepDominatorV);                    
      Bldr.generateNode(B, *it, state);                                    
      continue;                                                            
    }                                                                      
  }                                                                        
}

[Charusso]

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

123 ↗

(On Diff #206227)

I have seen we are producing tons of Unknowns and I am still not sure where they go, but even if I remove that condition these Unknowns will not shown up in the ExplodedGraph. I believe that should be the correct behavior.

[NoQ]

Nice!~ I'm glad this is getting sorted out.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

103 ↗

(On Diff #206227)

How about the following test case in which not Bar but &Bar gets bitwise-operated upon?

C **test() {
  C *Bar = new C;
  C **Baz = &Bar;
  Baz = reinterpret_cast<C **>(reinterpret_cast<uintptr_t>(Baz) | 0x1);
  Baz = reinterpret_cast<C **>(reinterpret_cast<uintptr_t>(Baz) & ~static_cast<uintptr_t>(0x1));
  delete *Baz;
}

The difference is that in this case the escaping region doesn't have a symbolic base. And i believe that symbolic regions aren't special here in any way.

I suggest doing an escape when the resulting value is unknown after evalBinOp but regardless of any other conditions that you mentioned. Simply because there's a loss of information.

clang/test/Analysis/symbol-escape.cpp

2 ↗

(On Diff #206227)

Pls include core as well, just in case, because running path-sensitive analysis without core checkers is unsupported.

5–6 ↗

(On Diff #206227)

I think something went wrong while uploading the patch. This diff should add this whole test file, not update it.

7 ↗

(On Diff #206227)

Relying on everybody's system headers is super flaky, don't do this in tests.

Please define stuff that you use directly like other tests do:

typedef unsigned __INTPTR_TYPE__ uintptr_t;

15 ↗

(On Diff #206227)

Bar is reduced to one bit here. It's a legit leak. I think you meant to swap Foo and Bar.

20 ↗

(On Diff #206227)

In any case, passing a pointer to delete that wasn't obtained from new is undefined behavior. In order to produce a test that's also a correct code, i think we should either undo our bitwise operations, or perform an escape in a different manner (say, return the pointer to the caller).

[Charusso]

• Fix.

[Charusso]

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

103 ↗

(On Diff #206227)

That is a great idea! I wanted to make it more generic. Thanks you!

clang/test/Analysis/symbol-escape.cpp

26 ↗

(On Diff #206332)

I have made it fail on master.

5–6 ↗

(On Diff #206227)

This is a test-driven-development-driven patch, first make the test fail, then make it pass.

7 ↗

(On Diff #206227)

It is a predefined header in Modules so Windows will not break.

Please note that only one of the test files (malloc.c) use that definition, most of them using that I have used in D62926, so I believe this header is the correct one as it is in four tests and Windows-approved by D62926.

15 ↗

(On Diff #206227)

I assumed that we are doing our low-bits magic, where we have two identical objects. The SymRegion sets up the HeapSymRegion's property, where no leak happen. We just could change the first low-bit and set up some crazy flag, like isChecked(), so we do not lost the tracking. Also we have four bits to change, so there is no edge case going on.

20 ↗

(On Diff #206227)

Whoops! I really wanted to make the test look great.

[NoQ]

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

123 ↗

(On Diff #206227)

Unknowns don't show up in the Environment, but they do show up in the Store.

clang/test/Analysis/symbol-escape.cpp

12 ↗

(On Diff #206332)

Let's make fancier names, like, dunno, test_escape_into_bitwise_ops and test_indirect_escape_into_bitwise_ops.

5–6 ↗

(On Diff #206227)

Sounds nice in your local git but for phabricator it's definitely better to upload the exact thing that you're going to commit. It is assumed that the tests would fail without the patch and will pass with the patch, and the information on how exactly did the tests fail previously would look nice in the description. Sometimes some (but usually not all) of the tests are added just because they were discovered to be nice tests related to the patch but not because they failed before the patch, which is also fine and should ideally be mentioned in the description.

7 ↗

(On Diff #206227)

Mmm, ok, if it's an internal file then i guess it might be non-flaky. But even then, if it's something more complicated than the definition of uintptr_t, then your test will be affected by the contents of this file, which may change. I'd slightly prefer to reduce the amount of such stuff in our tests simply for keeping them minimal.

15 ↗

(On Diff #206227)

I guess just flip a single bit like i did in my test?

[Charusso]

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

123 ↗

(On Diff #206227)

Oh, I really wanted to see them in the Environment. Thanks for the clarification!

clang/test/Analysis/symbol-escape.cpp

5–6 ↗

(On Diff #206227)

Lesson learned, thanks for the idea!

7 ↗

(On Diff #206227)

Hm, I hope that malloc.c one works.

[NoQ]

Great, thanks!

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

104 ↗

(On Diff #206341)

Pls put the comment inside else.

[Charusso]

• Better comment.

[Charusso]

Thanks for the review ! That was the largest-work three lines of code of the history because I was blind.

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp

if (!B->isAssignmentOp()) {
       if (B->getOpcode() == BO_PtrMemD)
         state = createTemporaryRegionIfNeeded(state, LCtx, LHS);
 
       // Process non-assignments except commas or short-circuited
       // logical expressions (LAnd and LOr).
       SVal Result = evalBinOp(state, Op, LeftV, RightV, B->getType());
       if (!Result.isUnknown()) {
         state = state->BindExpr(B, LCtx, Result);
+      } else {
+        // If we cannot evaluate the operation escape the operands.
+        state = escapeValue(state, LeftV, PSK_EscapeOther);
+        state = escapeValue(state, RightV, PSK_EscapeOther);
       }
 
       Bldr.generateNode(B, *it, state);
       continue;
     }
 
     assert (B->isCompoundAssignmentOp());
 

cfe/trunk/test/Analysis/symbol-escape.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,cplusplus.NewDeleteLeaks \
+// RUN:  -verify %s
+
+// expected-no-diagnostics: Whenever we cannot evaluate an operation we escape
+//                          the operands. After the evaluation it would be an
+//                          Unknown value and the tracking would be lost.
+
+typedef unsigned __INTPTR_TYPE__ uintptr_t;
+
+class C {};
+
+C *simple_escape_in_bitwise_op(C *Foo) {
+  C *Bar = new C();
+  Bar = reinterpret_cast<C *>(reinterpret_cast<uintptr_t>(Bar) & 0x1);
+  (void)Bar;
+  // no-warning: "Potential leak of memory pointed to by 'Bar'" was here.
+
+  return Bar;
+}
+
+C **indirect_escape_in_bitwise_op() {
+  C *Qux = new C();
+  C **Baz = &Qux;
+  Baz = reinterpret_cast<C **>(reinterpret_cast<uintptr_t>(Baz) | 0x1);
+  Baz = reinterpret_cast<C **>(reinterpret_cast<uintptr_t>(Baz) &
+                   ~static_cast<uintptr_t>(0x1));
+  // no-warning: "Potential leak of memory pointed to by 'Qux'" was here.
+
+  delete *Baz;
+  return Baz;
+}
+