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

via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 5 07:33:09 PST 2024


mzyKi 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 (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.
> 
> ```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 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:
```cpp
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?

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


More information about the cfe-commits mailing list