[clang] [analyzer] Fix crash on dereference invalid return value of getAdjustedParameterIndex() (PR #83585)

via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 4 23:19:10 PST 2024


tomasz-kaminski-sonarsource wrote:

> 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 (old C++ member methods)
c) explicit object member functions (`this` parameters)
d) static member functions (you can do `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 implicit object param mapped to this, and we need to expose 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 queue 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.
```C++
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 to fix it. 


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


More information about the cfe-commits mailing list