Skip to content

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

Merged
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:dev/fix-use-after-move-posix-source
Nov 8, 2024
Merged

fix(libutil/posix-source-accessor.cc): get rid of use-after-move bug#11837
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:dev/fix-use-after-move-posix-source

Conversation

@xokdvium
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium 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.

@xokdvium xokdvium requested a review from edolstra as a code owner November 8, 2024 13:05
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].
PosixSourceAccessor::PosixSourceAccessor(std::filesystem::path && root)
: root(std::move(root))
PosixSourceAccessor::PosixSourceAccessor(std::filesystem::path && argRoot)
: root(std::move(argRoot))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C-- :(

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Nov 8, 2024

Adding clang-tidy should now be easier with meson because we get a compile_commands.json for free.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Nov 8, 2024

@mergify queue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Nov 8, 2024

queue

🟠 Waiting for conditions to match

Details
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-skipped = installer_test
        • check-neutral = installer_test
        • check-success = installer_test
      • any of: [🛡 GitHub branch protection]
        • check-success = tests (ubuntu-latest)
        • check-neutral = tests (ubuntu-latest)
        • check-skipped = tests (ubuntu-latest)
      • any of: [🛡 GitHub branch protection]
        • check-success = tests (macos-latest)
        • check-neutral = tests (macos-latest)
        • check-skipped = tests (macos-latest)

@xokdvium
Copy link
Copy Markdown
Contributor Author

xokdvium commented Nov 8, 2024

@Mic92 I have filed the #11839 to track static analysis separately from sanitizers. I think that this should be a no-brainer, but since this is a potentially big change I'd like to gather some feedback on the idea first.

Mic92 added a commit that referenced this pull request Nov 8, 2024
…1837

fix(libutil/posix-source-accessor.cc): get rid of use-after-move bug (backport #11837)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants