[analyzer] Fix crash on dereference invalid return value of getAdjustedParameterIndex()

[mzyKi]

fix #78810
thanks for @Snape3058 's comment

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Exile (mzyKi)

Changes

fix #78810

---

Full diff: https://github.com/llvm/llvm-project/pull/83585.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+7-2)
• (added) clang/test/Analysis/engine/expr-engine-cxx-crash.cpp (+17)

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 504fd7f05e0f99..dc72945d68d56f 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -354,8 +354,13 @@ SVal ExprEngine::computeObjectUnderConstruction(
         // Operator arguments do not correspond to operator parameters
         // because this-argument is implemented as a normal argument in
         // operator call expressions but not in operator declarations.
-        const TypedValueRegion *TVR = Caller->getParameterLocation(
-            *Caller->getAdjustedParameterIndex(Idx), BldrCtx->blockCount());
+        std::optional<unsigned int> Index =
+            Caller->getAdjustedParameterIndex(Idx);
+        if (!Index) {
+          return std::nullopt;
+        }
+        const TypedValueRegion *TVR =
+            Caller->getParameterLocation(*Index, BldrCtx->blockCount());
         if (!TVR)
           return std::nullopt;
 
diff --git a/clang/test/Analysis/engine/expr-engine-cxx-crash.cpp b/clang/test/Analysis/engine/expr-engine-cxx-crash.cpp
new file mode 100644
index 00000000000000..fa0718668a75be
--- /dev/null
+++ b/clang/test/Analysis/engine/expr-engine-cxx-crash.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core.DivideZero -std=c++23 -verify %s
+// expected-no-diagnostics
+
+struct S
+{
+    constexpr auto operator==(this auto, S)
+    {
+        return true;
+    }
+};
+
+int main()
+{
+    return S {} == S {};
+}
+
+// test
\ No newline at end of file

[Snape3058]

Add suggested reviewers for this PR. I am unfamiliar with the engine code.

mzyKi force-pushed the bugfix/crash-on-getAdjustedParameterIndex branch

Please do not force push to your forked repo later after receiving suggestions from other reviewers. This will make previous suggestions hard to track. We have the squash and merge functionality to merge all the commits on your branch into one commit in the LLVM repo when everything is done.

[steakhal]

I feel like it's better than crashing, but I suspect it only resolves that, right?

[tomasz-kaminski-sonarsource]

Thank you for creating the ticket, and fixing the issue. I am a bit concerned that the fix is hiding the symptoms and not the actual bug. After a bit of checking, I think the issue is caused by the fact that for the explicit object functions, we are creating a CXXMemberOperatorCall event, that performs parameter adjustment.
The corresponding code can be found here:

 if (const auto *OpCE = dyn_cast<CXXOperatorCallExpr>(CE)) {
    const FunctionDecl *DirectCallee = OpCE->getDirectCallee();
    if (const auto *MD = dyn_cast<CXXMethodDecl>(DirectCallee))
      if (MD->isInstance())  // <-- The root of the issue
        return create<CXXMemberOperatorCall>(OpCE, State, LCtx, ElemRef);
  }

I think it would be fixed by replacing MD->isInstance() with MD->isImplicitObjectMemberFunction(). This will make use fallback to SimpleFunctionCall, which I think is desired, as for example such calls do not have thisRegion.

That would make the behavior consistent with the behavior for other explicit object functions (deducing this).
To illustrate we have the following code, the AST and produced CallEvent are different:

struct Foo {
  void foo();
  void bar(this Foo&);
};

void test(Foo f) {
    f.foo(); // represented as `CXXMemberCallExpr`, and creates`CXXMemberCall` event
    f.bar(); // represented as `CallExpr`, and creates `SimpleFunctionCall` event
}

Live link here: https://godbolt.org/z/sjEbb6rMe.

Would you be willing to adjust PR to implement the suggested change?

[mzyKi]

That would make the behavior consistent with the behavior for other explicit object functions (deducing this).
To illustrate we have the following code, the AST and produced CallEvent are different:

Thanks for your help very much.I only fixed the symptoms of this bug without spending much time to find the root cause. I feel ashamed. But I am not sure whether this change has some side effects.MD->isInstance() replaced with MD->isImplicitObjectMemberFunction() will miss some cases create <CXXMemberOperatorCall> in my opinion.Are these missing cases unnecessary?

[tomasz-kaminski-sonarsource]

Thanks for your help very much.I only fixed the symptoms of this bug without spending much time to find the root cause. I feel ashamed.

There is nothing to be ashamed of. You have spent the time to debug the crash, and localize the use of non-engaged optional, this is already helpful for the project. And addressing the crash (for example by stopping analysis) makes sure, that analysis continues and we are able to report issues from other functions. This is an improvement over the status quo, and sometimes determining the root cause is very difficult.

But I am not sure whether this change has some side effects.MD->isInstance() replaced with MD->isImplicitObjectMemberFunction() will miss some cases create <CXXMemberOperatorCall> in my opinion.Are these missing cases unnecessary?

There are 4 possible choices on how an operator can be overloaded in C++:
a) free functions, including friends
b) implicit object member functions (plain old C++ member functions)
c) explicit object member functions (available since C++23, using this parameters)
d) static member functions (available since C++23, for operator() and operator[])

When the CXXOperatorCallExpr is constructed, it will always contain an object parameter on the argument list. So for the argument to parameter adjustment, we need to do:
a) nothing -> same number of params and args
b) adjust + 1 -> there is an implicit object param mapped to this, and we need to expose information about object pointed by this region
c) nothing -> object has the corresponding param, so the number is the same
d) this is a bit tricky, as a(10) will have 2 args and only one parameter.

Before C++23, the option didn't exist, and isInstance() implied that we are in case (b). Now isInstance() covers both b and c, which is leading to the crash you have found. This we need to fallback to the old status quo by using isImplicitObjectParameter().

We may still not cover the case d, of static operators, for example, we should verify if div by zero is raised here if the operator is static, and if removing static helps. But I think this should be handled as a separate PR.

struct C {
  static void operator()(int x) { return 10 / x; }
};

void test(C c) {
   c(10);
}

If you would be interested in looking at also fixing it after this PR, I would be happy to provide my thoughts, on how I would approach it.

[mzyKi]

If you would be interested in looking at also fixing it after this PR, I would be happy to provide my thoughts, on how I would approach it.

Thanks for your explanation and I'm willing to do the work later.I also hope that you will give me some suggestions about that.
I test the following code:

struct C {
  static int operator()(int x) { return 10 / x; }
};

void test(C c) {
   c(0);
}

run command clang-tidy -checks=-*,clang-analyzer-core.DivideZero test.cpp -- -std=c++23 and it show me nothing detected.
We need to fix it after? @tomasz-kaminski-sonarsource

[steakhal]

The patch looks a lot better this way. Thanks!

I'd recommend adding this test to the existing clang/test/Analysis/cxx2b-deducing-this.cpp test file to have all deducing this testcases at a common place.

[tomasz-kaminski-sonarsource]

While moving the test case to cxx2b-deducing-this.cpp it would be also nice to add a clang_analyzer_dump() line similar to one already existing in the file, to see if the values of the arguments are mapped correctly.

We need to fix it after? @tomasz-kaminski-sonarsource

This is separate from deducing this, so let's put in in a separate PR, and finish this first.
I will post a more detailed explanation later today or tomorrow.

[tomasz-kaminski-sonarsource]

Looking at the examples for the static operator at (live example):

struct S {
  int pad;  
  static void operator()(int x, int y) {
    // clang_analyzer_dump(x);
    // clang_analyzer_dump(y); 
  }
};

void calls() {
    S s{10};
    void (*fptr)(int, int) = &S::operator();

    s(1, 2);  // A
    S::operator()(1, 2);  // B
    fptr(1, 2); // C
}

You can experiment with the results of the dumps to check current behavior - you need to add debug.ExprInspection matcher to see the output.

The calls B and C are represented as the CallExpr with 2 arguments, and in static analysis, they are mapped to SimpleFunctionCall event. This is no different from handling of any other static member function, that we have currently.

However, in the case of A the AST is different, as we have CXXOperatorCallExpr with 3 arguments being included (the first node is called function). The additional argument is the object argument, which is not later passed to function because it is static.

CXXOperatorCallExpr <line:13:5, col:11> 'void' '()'
|-ImplicitCastExpr <col:6, col:11> 'void (*)(int, int)' <FunctionToPointerDecay>
| `-DeclRefExpr <col:6, col:11> 'void (int, int)' lvalue CXXMethod 0xc7ecd78 'operator()' 'void (int, int)'
|-DeclRefExpr <col:5> 'S' lvalue Var 0xc7ed028 's' 'S'
|-IntegerLiteral <col:7> 'int' 1
`-IntegerLiteral <col:10> 'int' 2

So in this case we also need an adjustment when mapping from arguments to parameters. However, we cannot use CXXMemberOperatorCall as we are not calling an implicit object function, and we do not have this argument.
I think the best way to handle it would be to introduce a new kind of event CXXStaticOperatorCall.
This event should be based on SimpleFunctionCall (I would start it as a copy of it), with some modification derived from CXXMemberOperatorCall:

• constructor from CXXOperatorCallExpr
• getOriginExpr and getOverloadedOperator as we will always use it for CXXOperatorCallExpr
• getNumArgs, getArgExpr, getAdjustedParameterIndex, and getASTArgumentIndex to skip object parameters.

[ Note: We do not copy any function related to this object, as there is not any. ]

Then we need to add a new CE_CXXStaticOperator kind, it should be after CE_Function and before CE_CXXMember as this is not an instance call.

enum CallEventKind {
  CE_Function,
  CE_CXXStaticOperator,
  CE_CXXMember,
  CE_CXXMemberOperator,
  CE_CXXDestructor,
  CE_BEG_CXX_INSTANCE_CALLS = CE_CXXMember,
  CE_END_CXX_INSTANCE_CALLS = CE_CXXDestructor,
  ....
};

Finally, we can change getSimpleCall to check for static operators:

 if (const auto *OpCE = dyn_cast<CXXOperatorCallExpr>(CE)) {
    const FunctionDecl *DirectCallee = OpCE->getDirectCallee();
    if (const auto *MD = dyn_cast<CXXMethodDecl>(DirectCallee)) {
      if (MD->isInstance()
        return create<CXXMemberOperatorCall>(OpCE, State, LCtx, ElemRef);
      if (MD->isStatic())
         return create<CXXStaticOperatorCall>(OpCE, State, LCtx, ElemRef);
     }    
  }

Hope this helps. However, please open such changes in a separate PR.

[steakhal]

Backporting this in PR #84194 to clang-18.