Skip to content

[clang-tidy] std::addressof not checked by clang-analyzer-core.StackAddressEscape #94193

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

Closed
dfrvfmst opened this issue Jun 3, 2024 · 4 comments · Fixed by #99886
Closed

[clang-tidy] std::addressof not checked by clang-analyzer-core.StackAddressEscape #94193

dfrvfmst opened this issue Jun 3, 2024 · 4 comments · Fixed by #99886

Comments

@dfrvfmst
Copy link

dfrvfmst commented Jun 3, 2024

The code examples are all in C++23 (-std=c++23) and libc++ (-stdlib=libc++ -fexperimental-library).


Given a basic example:

#include <iostream>
#include <memory>
#include <string>

auto f1()
{
    std::string a = "123";
    return std::addressof(a);
}

int main()
{
    auto result = f1();
    std::cout << *result;
}

It isn't caught by clang-tidy, and instead ASan catches it in runtime:

==270312==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f5177600020 at pc 0x557cec62fff6 bp 0x7ffcf239dcb0 sp 0x7ffcf239dca8

This is weird, given the fact that __builtin_addressof is checked by clang-tidy and (at least in libc++) std::addressof is simply a wrapper that calls __builtin_addressof.

Definition of std::addressof:

template <class _Tp>
inline _LIBCPP_CONSTEXPR_SINCE_CXX17 _LIBCPP_NO_CFI _LIBCPP_HIDE_FROM_ABI _Tp* addressof(_Tp& __x) _NOEXCEPT {
  return __builtin_addressof(__x);
}

When changing the example to use __builtin_addressof:

auto f1()
{
    std::string a = "123";
    return __builtin_addressof(a);
}

clang-tidy is able to figure out that it's returning a dangling pointer:

main.cpp:38: error: Address of stack memory associated with local variable 'a' returned to caller [clang-analyzer-core.StackAddressEscape,-warnings-as-errors]

This issue might be what caused std::reference_wrapper and std::ranges::ref_view to be unchecked, since (in libc++) they also used std::addressof in their constructors.

Example of faulty code utilizing std::ranges::ref_view:

#include <iostream>
#include <ranges>
#include <vector>

auto f2()
{
    std::vector a{1, 2, 3};
    return a | std::ranges::views::filter([](auto &&_) { return _ > 1; });
}

int main()
{
    auto result = f2();
    for (auto &&e : result) {
        std::cout << e;
    }
}

clang-tidy doesn't catch it. ASan catches it (stack-use-after-return).

Example of faulty code utilizing std::reference_wrapper:

#include <iostream>
#include <functional>
#include <string>

auto f3()
{
    std::string a = "123";
    return std::ref(a);
}

int main()
{
    auto result = f3();
    std::cout << result.get();
}

clang-tidy doesn't catch it. ASan catches it (stack-use-after-return).


More interestingly, if you implement your own reference wrapper that works similar to the one in libc++ by storing address, clang-tidy can detect the returning of dangling pointer/reference as long as std::addressof isn't used in constructor.

Definition of Ref:

template<typename T>
struct Ref
{
    Ref(T &ref)
        : ptr(&ref)
    {}
    auto &get(this auto &&self) { return *self.ptr; }
private:
    T *ptr;
};

Faulty code utilizing Ref:

#include <iostream>
#include <string>

auto f4()
{
    std::string a = "123";
    return Ref{a};
}

int main()
{
    auto result = f4();
    std::cout << result.get();
}

clang-tidy catches it:

main.cpp:28: error: Address of stack memory associated with local variable 'a' is still referred to by the stack variable 'result' upon returning to the caller.  This will be a dangling reference [clang-analyzer-core.StackAddressEscape,-warnings-as-errors]

But, when you change the constructor of Ref to use std::addressof(ref), clang-tidy can no longer detect it, which leads to me thinking the case with std::reference_wrapper and std::ranges::ref_view.


Version:

> clang-tidy-18 --version
Debian LLVM version 18.1.6
  Optimized build.
> clang++-18 --version
Debian clang version 18.1.6 (++20240518023138+1118c2e05e67-1~exp1~20240518143226.133)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
@llvmbot
Copy link
Member

llvmbot commented Jun 3, 2024

@llvm/issue-subscribers-clang-static-analyzer

Author: None (dfrvfmst)

The code examples are all in C++23 (`-std=c++23`) and libc++ (`-stdlib=libc++ -fexperimental-library`).

Given a basic example:

#include &lt;iostream&gt;
#include &lt;memory&gt;
#include &lt;string&gt;

auto f1()
{
    std::string a = "123";
    return std::addressof(a);
}

int main()
{
    auto result = f1();
    std::cout &lt;&lt; *result;
}

It isn't caught by clang-tidy, and instead ASan catches it in runtime:

==270312==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f5177600020 at pc 0x557cec62fff6 bp 0x7ffcf239dcb0 sp 0x7ffcf239dca8

This is weird, given the fact that __builtin_addressof is checked by clang-tidy and (at least in libc++) std::addressof is simply a wrapper that calls __builtin_addressof.

Definition of std::addressof:

template &lt;class _Tp&gt;
inline _LIBCPP_CONSTEXPR_SINCE_CXX17 _LIBCPP_NO_CFI _LIBCPP_HIDE_FROM_ABI _Tp* addressof(_Tp&amp; __x) _NOEXCEPT {
  return __builtin_addressof(__x);
}

When changing the example to use __builtin_addressof:

auto f1()
{
    std::string a = "123";
    return __builtin_addressof(a);
}

clang-tidy is able to figure out that it's returning a dangling pointer:

main.cpp:38: error: Address of stack memory associated with local variable 'a' returned to caller [clang-analyzer-core.StackAddressEscape,-warnings-as-errors]

This issue might be what caused std::reference_wrapper and std::ranges::ref_view to be unchecked, since (in libc++) they also used std::addressof in their constructors.

Example of faulty code utilizing std::ranges::ref_view:

#include &lt;iostream&gt;
#include &lt;ranges&gt;
#include &lt;vector&gt;

auto f2()
{
    std::vector a{1, 2, 3};
    return a | std::ranges::views::filter([](auto &amp;&amp;_) { return _ &gt; 1; });
}

int main()
{
    auto result = f2();
    for (auto &amp;&amp;e : result) {
        std::cout &lt;&lt; e;
    }
}

clang-tidy doesn't catch it. ASan catches it (stack-use-after-return).

Example of faulty code utilizing std::reference_wrapper:

#include &lt;iostream&gt;
#include &lt;functional&gt;
#include &lt;string&gt;

auto f3()
{
    std::string a = "123";
    return std::ref(a);
}

int main()
{
    auto result = f3();
    std::cout &lt;&lt; result.get();
}

clang-tidy doesn't catch it. ASan catches it (stack-use-after-return).


More interestingly, if you implement your own reference wrapper that works similar to the one in libc++ by storing address, clang-tidy can detect the returning of dangling pointer/reference as long as std::addressof isn't used in constructor.

Definition of Ref:

template&lt;typename T&gt;
struct Ref
{
    Ref(T &amp;ref)
        : ptr(&amp;ref)
    {}
    auto &amp;get(this auto &amp;&amp;self) { return *self.ptr; }
private:
    T *ptr;
};

Faulty code utilizing Ref:

#include &lt;iostream&gt;
#include &lt;string&gt;

auto f4()
{
    std::string a = "123";
    return Ref{a};
}

int main()
{
    auto result = f4();
    std::cout &lt;&lt; result.get();
}

clang-tidy catches it:

main.cpp:28: error: Address of stack memory associated with local variable 'a' is still referred to by the stack variable 'result' upon returning to the caller.  This will be a dangling reference [clang-analyzer-core.StackAddressEscape,-warnings-as-errors]

But, when you change the constructor of Ref to use std::addressof(ref), clang-tidy can no longer detect it, which leads to me thinking the case with std::reference_wrapper and std::ranges::ref_view.


Version:

&gt; clang-tidy-18 --version
Debian LLVM version 18.1.6
  Optimized build.
&gt; clang++-18 --version
Debian clang version 18.1.6 (++20240518023138+1118c2e05e67-1~exp1~20240518143226.133)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

@steakhal
Copy link
Contributor

steakhal commented Jun 3, 2024

Thanks for the detailed report.
I suspect this is the same issue for std::addressof as I mentioned in #90174 (comment), that we don't inline the std::addressof thus we never see that it is just a builtin_addressof call.

@dfrvfmst
Copy link
Author

A somewhat relevant example that uses string_view: https://godbolt.org/z/8d5r5nMbG

If you replace the std::string_view with your own clone, the dangling problem is reliably detected no matter how you try to obfuscate it. Otherwise it suffers from a similar issue as #68637 and #63310. It seems as if clang-tidy treats stdlib rather differently in the sense that functions and constructors are seen as opaque operations and are handled case-by-case, even though it should be theoretically able to detect the problems by reusing the same general analyzer code path.

@dfrvfmst
Copy link
Author

Is this a good first issue? What kind of skills are required to diagnose and patch it?

steakhal added a commit that referenced this issue Jul 23, 2024
Some template function instantiations don't have a body, even though
their templates did have a body.
Examples are: `std::move`, `std::forward`, `std::addressof` etc.

They had bodies before
72315d0

After that change, the sentiment was that these special functions should
be considered and treated as builtin functions.

Fixes #94193

CPP-5358
yuxuanchen1997 pushed a commit that referenced this issue Jul 25, 2024
Summary:
Some template function instantiations don't have a body, even though
their templates did have a body.
Examples are: `std::move`, `std::forward`, `std::addressof` etc.

They had bodies before
72315d0

After that change, the sentiment was that these special functions should
be considered and treated as builtin functions.

Fixes #94193

CPP-5358

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251093
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants