Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(libutil/posix-source-accessor.cc): get rid of use-after-move bug (backport #11837) #11838

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Nov 8, 2024

Naming class member variables the same as constructor arguments is a very slippery slope because of how member variable names get resolved. Compiler is not very helpful here and we need static analysis to forbid this kind of stuff.

The following example illustrates the cause quite well:

struct B {
    B(int) {}
};

struct A {
    A(int b): b([&](){
        return b;
        static_assert(std::is_same_v<decltype(b), int>);
    }()) {
       static_assert(std::is_same_v<decltype(b), int>);
    }
    void member() {
        static_assert(std::is_same_v<decltype(b), B>);
    }
    B b;
};

int main() {
    A(1).member();
}

From N4861 6.5.1 Unqualified name lookup:

In all the cases listed in [basic.lookup.unqual], the scopes are searched
for a declaration in the order listed in each of the respective categories;
name lookup ends as soon as a declaration is found for the name.
If no declaration is found, the program is ill-formed.

In the affected code there was a use-after-move for all accessses in the constructor body, but this UB wasn't triggered.

These types of errors are trivial to catch via clang-tidy's [clang-analyzer-cplusplus.Move].

Motivation

UB is bad.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.


This is an automatic backport of pull request #11837 done by [Mergify](https://mergify.com).

Naming class member variables the same as constructor arguments is a very
slippery slope because of how member variable names get resolved. Compiler
is not very helpful here and we need static analysis to forbid this kind of
stuff.

The following example illustrates the cause quite well:

```cpp

struct B {
    B(int) {}
};

struct A {
    A(int b): b([&](){
        return b;
        static_assert(std::is_same_v<decltype(b), int>);
    }()) {
       static_assert(std::is_same_v<decltype(b), int>);
    }
    void member() {
        static_assert(std::is_same_v<decltype(b), B>);
    }
    B b;
};

int main() {
    A(1).member();
}
```

From N4861 6.5.1 Unqualified name lookup:

> In all the cases listed in [basic.lookup.unqual], the scopes are searched
> for a declaration in the order listed in each of the respective categories;
> name lookup ends as soon as a declaration is found for the name.
> If no declaration is found, the program is ill-formed.

In the affected code there was a use-after-move for all accesses in the constructor
body, but this UB wasn't triggered.

These types of errors are trivial to catch via clang-tidy's [clang-analyzer-cplusplus.Move].

(cherry picked from commit 3e0129c)
@mergify mergify bot requested a review from edolstra as a code owner November 8, 2024 13:58
@mergify mergify bot added the merge-queue label Nov 8, 2024
@Mic92 Mic92 merged commit ef21dfa into 2.24-maintenance Nov 8, 2024
22 of 24 checks passed
@Mic92 Mic92 deleted the mergify/bp/2.24-maintenance/pr-11837 branch November 8, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants