clang-tidy: cppcoreguidelines-owning-memory doesn't flag double owning

[denzor200]

    gsl::owner<int*> Owner1 = new int(42);
    gsl::owner<int*> Owner2 = Owner1;
    // Owner1 = nullptr;

This may lead to double free because following the core guidelines rules we must call delete Owner1; and delete Owner2; later.

Another interesting double owning case is regarding move constructors and move assignments:

class Foo {
    gsl::owner<int*> ptr=nullptr;
public:
    ~Foo() {
        delete ptr;
    }
    Foo(Foo&& r) : ptr(r.ptr) {
        // r.ptr = nullptr;
    }
    Foo& operator=(Foo&& r) {
        if (&r != this) {
            ptr = r.ptr;
            // r.ptr = nullptr;
        }
        return *this;
    }
};

In practice, such kind of wrong code mostly reached due to the fact that people expect std::move to set it's pointer argument to nullptr but it never do that:

    Foo(Foo&& r) : ptr(std::move(r.ptr)) { // BAD
    }
    Foo& operator=(Foo&& r) {
        if (&r != this) {
            ptr = std::move(r.ptr);        // BAD
        }
        return *this;
    }

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Denis Mikhailov (denzor200)

``` gsl::owner<int*> Owner1 = new int(42); gsl::owner<int*> Owner2 = Owner1; // Owner1 = nullptr; ```

This may lead to double free because following the core guidelines rules we must call delete Owner1; and delete Owner2; later.

Another interesting double owning case is regarding move constructors and move assignments:

class Foo {
    gsl::owner<int*> ptr=nullptr;
public:
    ~Foo() {
        delete ptr;
    }
    Foo(Foo&& r) : ptr(r.ptr) {
        // r.ptr = nullptr;
    }
    Foo& operator=(Foo&& r) {
        if (&r != this) {
            ptr = r.ptr;
            // r.ptr = nullptr;
        }
        return *this;
    }
};

In practice, such kind of wrong code mostly reached due to the fact that people expect std::move to set it's pointer argument to nullptr but it never do that:

    Foo(Foo&& r) : ptr(std::move(r.ptr)) { // BAD
    }
    Foo& operator=(Foo&& r) {
        if (&r != this) {
            ptr = std::move(r.ptr);        // BAD
        }
        return *this;
    }

[vbvictor]

The first example with Owner1 and Owner2 is described at the end of the check as a limitation. Without major rewrite of the check to use flow-sensitive analysis, it's not possible to detect such cases. I think flow-sensitive analysis should be implemented in CSA

[5chmidti]

The first example with Owner1 and Owner2 is described at the end of the check as a limitation. Without major rewrite of the check to use flow-sensitive analysis, it's not possible to detect such cases. I think flow-sensitive analysis should be implemented in CSA

Checks that are flow-sensitive can also use the data flow framework instead of the CSA framework

[vbvictor]

The first example with Owner1 and Owner2 is described at the end of the check as a limitation. Without major rewrite of the check to use flow-sensitive analysis, it's not possible to detect such cases. I think flow-sensitive analysis should be implemented in CSA

Checks that are flow-sensitive can also use the data flow framework instead of the CSA framework

Yes, but performance-wise it probably be better to use CSA framework. In CSA a large portion of time is accounted for core part, so having N+1 checks instead of N may not be a huge difference. But for clang-tidy, there aren't many checks that use flow-sensitive analysis, and I'm not sure if flow-sensitive state can be shared across them, so each check would add a big runtime overhead. This can be a premature optimization from me, so take with doubt.

[5chmidti]

I've never looked at the CSA implementation, so I didn't know that. From the data flow framework I know that state is not shared across them. It's essentially AST matchers plus a framework to transition states and doing satisfyability checks based on visiting those matches

[NagyDonat]

I think flow-sensitive analysis should be implemented in CSA

Based on my familiarity with the internal architecture of the analyzer, I don't think that this is feasible.

The analyzer does have marginal support for merging execution paths if they reach a certain program point with exactly the same state -- but even this trivial merge is a source of bugs and headaches, because some parts of the analyzer blindly follow the first parent.

On a theoretical level, flow-sensitive analysis would be a combination of this "merge if states are identical" step with a heuristic step that simplifies the state and ensures that the differences between the states are eliminated. Unfortunately in the static analyzer the execution state is represented by a very complex data structure, which incorporates "traits" that are added by the individual checkers (e.g. StreamChecker registers a state trait which tracks the files that are opened/closed -- this is how all the different checkers can share the same symbolic execution). To implement state simplification we would need to implement a "simplify your state traits" step in many individual checkers, which would be a massive code change.

Moreover, the state simplification step (a.k.a. discarding data about the differences between the execution paths) would naturally increase the amount of false positives, so introducing flow-sensitive analysis to CSA would be a complex balancing act where we'd need to stomach losses and re-stabilize the checkers that are currently stable and used by many users.

Yes, but performance-wise it probably be better to use CSA framework. In CSA a large portion of time is accounted for core part, so having N+1 checks instead of N may not be a huge difference.

You're right that in CSA we have good (or at least acceptable...) performance due to the design that all the individual checkers rely on the same shared modeling which is done by the core engine -- but the downside is that the engine of the static analyzer needs to remain compatible with all the different checkers, so it's very difficult to change its behavior (especially with drastic changes like fundamentally changing the flow of execution).