[PATCH] Catch use-after-free bugs in usage of shared ptrs using Clang's consumed attributes

[Raphael S. Carvalho]

Clang provides consumed annotations, which are very useful for
statically catching use-after-free bugs in specific classes.

That's done by:
1) Marking class as consumable with: clang::consumable().
2) Annotates contructors with clang::return_typestate(), to inform
new objects are in "unconsumed" state.
3) Set state of objects moved as "consumed" using clang::set_typestate()
4) Methods that access the object will be annotated with
clang::callable_when(unconsumed).

This way, clang screams whenever a code path tries to access an
object that was moved, i.e. has consumed state.

For years, we have been suffering with user-after-free bug around
smart pointers.

Therefore, both shared_ptr and lw_shared_ptr will now benefit from
this static analysis (not enabled by default).

To enable it, one must use clang, of course, and also provide the
-Wconsumed flag. As the analysis can report false positives,
it's important to disable -Werror (possibly by overriding it with
-Wno-error), so to allow the compilation to run to its completion.

This tool was able to find 4 use-after-free bugs in Scylla so far.

Example of report by clang once it hits a possible use-after-free:
"sstables/mx/reader.cc:1705:58: error: invalid invocation of method 'operator*'
on object 'schema' while it is in the 'consumed' state [-Werror,-Wconsumed]
legacy_reverse_slice_to_native_reverse_slice(*schema, slice.get()),
pc, std::move(trace_state), fwd, fwd_mr, monitor);"

Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>
---
include/seastar/core/shared_ptr.hh | 56 +++++++++++++++----
include/seastar/util/consumable-attributes.hh | 38 +++++++++++++
2 files changed, 82 insertions(+), 12 deletions(-)
create mode 100644 include/seastar/util/consumable-attributes.hh

diff --git a/include/seastar/core/shared_ptr.hh b/include/seastar/core/shared_ptr.hh
index e2e02640..7d3c2c0b 100644
--- a/include/seastar/core/shared_ptr.hh
+++ b/include/seastar/core/shared_ptr.hh
@@ -24,6 +24,7 @@
#include <seastar/core/shared_ptr_debug_helper.hh>
#include <seastar/util/is_smart_ptr.hh>
#include <seastar/util/indirect.hh>
+#include <seastar/util/consumable-attributes.hh>
#include <boost/intrusive/parent_from_member.hpp>
#include <functional>
#include <memory>
@@ -258,23 +259,30 @@ struct lw_shared_ptr_accessors<T, void_t<decltype(lw_shared_ptr_deleter<T>{})>>
}

template <typename T>
-class lw_shared_ptr {
+class SEASTAR_CONSUMABLE(unconsumed) lw_shared_ptr {
template <typename U>
using accessors = internal::lw_shared_ptr_accessors<std::remove_const_t<U>>;

mutable lw_shared_ptr_counter_base* _p = nullptr;
private:
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
lw_shared_ptr(lw_shared_ptr_counter_base* p) noexcept : _p(p) {
if (_p) {
++_p->_count;
}
}
template <typename... A>
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
static lw_shared_ptr make(A&&... a) {
auto p = new typename accessors<T>::concrete_type(std::forward<A>(a)...);
accessors<T>::instantiate_to_value(p);
return lw_shared_ptr(p);
}
+
+ SEASTAR_SET_TYPESTATE(consumed)
+ void invalidate() noexcept {
+ _p = nullptr;
+ }
public:
using element_type = T;

@@ -292,8 +300,11 @@ class lw_shared_ptr {
}
};

+ SEASTAR_RETURN_TYPESTATE(unconsumed)
lw_shared_ptr() noexcept = default;
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
lw_shared_ptr(std::nullptr_t) noexcept : lw_shared_ptr() {}
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
lw_shared_ptr(const lw_shared_ptr& x) noexcept : _p(x._p) {
if (_p) {
#pragma GCC diagnostic push
@@ -304,8 +315,9 @@ class lw_shared_ptr {
#pragma GCC diagnostic pop
}
}
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
lw_shared_ptr(lw_shared_ptr&& x) noexcept : _p(x._p) {
- x._p = nullptr;
+ x.invalidate();
}
[[gnu::always_inline]]
~lw_shared_ptr() {
@@ -341,8 +353,11 @@ class lw_shared_ptr {
return *this;
}

+ SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(unconsumed)
T& operator*() const noexcept { return *accessors<T>::to_value(_p); }
+ SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(unconsumed)
T* operator->() const noexcept { return accessors<T>::to_value(_p); }
+ SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(unconsumed)
T* get() const noexcept {
if (_p) {
return accessors<T>::to_value(_p);
@@ -376,6 +391,7 @@ class lw_shared_ptr {
}
}

+ SEASTAR_RETURN_TYPESTATE(unconsumed)
operator lw_shared_ptr<const T>() const noexcept {
return lw_shared_ptr<const T>(_p);
}
@@ -499,28 +515,40 @@ class enable_shared_from_this : private shared_ptr_count_base {
};

template <typename T>
-class shared_ptr {
+class SEASTAR_CONSUMABLE(unconsumed) shared_ptr {
mutable shared_ptr_count_base* _b = nullptr;
mutable T* _p = nullptr;
private:
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
explicit shared_ptr(shared_ptr_count_for<T>* b) noexcept : _b(b), _p(&b->data) {
++_b->count;
}
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr(shared_ptr_count_base* b, T* p) noexcept : _b(b), _p(p) {
if (_b) {
++_b->count;
}
}
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
explicit shared_ptr(enable_shared_from_this<std::remove_const_t<T>>* p) noexcept : _b(p), _p(static_cast<T*>(p)) {
if (_b) {
++_b->count;
}
}
+
+ SEASTAR_SET_TYPESTATE(consumed)
+ void invalidate() noexcept {
+ _b = nullptr;
+ _p = nullptr;
+ }
public:
using element_type = T;

+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr() noexcept = default;
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr(std::nullptr_t) noexcept : shared_ptr() {}
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr(const shared_ptr& x) noexcept
: _b(x._b)
, _p(x._p) {
@@ -528,13 +556,14 @@ class shared_ptr {
++_b->count;
}
}
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr(shared_ptr&& x) noexcept
: _b(x._b)
, _p(x._p) {
- x._b = nullptr;
- x._p = nullptr;
+ x.invalidate();
}
template <typename U, typename = std::enable_if_t<std::is_base_of<T, U>::value>>
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr(const shared_ptr<U>& x) noexcept
: _b(x._b)
, _p(x._p) {
@@ -543,11 +572,11 @@ class shared_ptr {
}
}
template <typename U, typename = std::enable_if_t<std::is_base_of<T, U>::value>>
+ SEASTAR_RETURN_TYPESTATE(unconsumed)
shared_ptr(shared_ptr<U>&& x) noexcept
: _b(x._b)
, _p(x._p) {
- x._b = nullptr;
- x._p = nullptr;
+ x.invalidate();
}
~shared_ptr() {
#pragma GCC diagnostic push
@@ -595,12 +624,15 @@ class shared_ptr {
explicit operator bool() const noexcept {
return _p;
}
+ SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(unconsumed)
T& operator*() const noexcept {
return *_p;
}
+ SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(unconsumed)
T* operator->() const noexcept {
return _p;
}
+ SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(unconsumed)
T* get() const noexcept {
return _p;
}
@@ -721,35 +753,35 @@ enable_shared_from_this<T>::shared_from_this() const noexcept {
template <typename T, typename U>
inline
bool
-operator==(const shared_ptr<T>& x, const shared_ptr<U>& y) {
+operator==(const shared_ptr<T>& x SEASTAR_PARAM_TYPESTATE(unconsumed), const shared_ptr<U>& y SEASTAR_PARAM_TYPESTATE(unconsumed)) {
return x.get() == y.get();
}

template <typename T>
inline
bool
-operator==(const shared_ptr<T>& x, std::nullptr_t) {
+operator==(const shared_ptr<T>& x SEASTAR_PARAM_TYPESTATE(unconsumed), std::nullptr_t) {
return x.get() == nullptr;
}

template <typename T>
inline
bool
-operator==(std::nullptr_t, const shared_ptr<T>& y) {
+operator==(std::nullptr_t, const shared_ptr<T>& y SEASTAR_PARAM_TYPESTATE(unconsumed)) {
return nullptr == y.get();
}

template <typename T>
inline
bool
-operator==(const lw_shared_ptr<T>& x, std::nullptr_t) {
+operator==(const lw_shared_ptr<T>& x SEASTAR_PARAM_TYPESTATE(unconsumed), std::nullptr_t) {
return x.get() == nullptr;
}

template <typename T>
inline
bool
-operator==(std::nullptr_t, const lw_shared_ptr<T>& y) {
+operator==(std::nullptr_t, const lw_shared_ptr<T>& y SEASTAR_PARAM_TYPESTATE(unconsumed)) {
return nullptr == y.get();
}

diff --git a/include/seastar/util/consumable-attributes.hh b/include/seastar/util/consumable-attributes.hh
new file mode 100644
index 00000000..de11ac2f
--- /dev/null
+++ b/include/seastar/util/consumable-attributes.hh
@@ -0,0 +1,38 @@
+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright (C) 2023 ScyllaDB
+ */
+
+#pragma once
+
+#ifdef __clang__
+ #define SEASTAR_CONSUMABLE(resource) [[clang::consumable(resource)]]
+ #define SEASTAR_CALLABLE_WHEN(condition) [[clang::callable_when(condition)]]
+ #define SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(condition) [[clang::callable_when(#condition, "unknown")]]
+ #define SEASTAR_RETURN_TYPESTATE(condition) [[clang::return_typestate(condition)]]
+ #define SEASTAR_SET_TYPESTATE(condition) [[clang::set_typestate(condition)]]
+ #define SEASTAR_PARAM_TYPESTATE(condition) [[clang::param_typestate(condition)]]
+#else
+ #define SEASTAR_CONSUMABLE(resource)
+ #define SEASTAR_CALLABLE_WHEN(condition)
+ #define SEASTAR_CALLABLE_WHEN_UNKNOWN_AND(condition)
+ #define SEASTAR_RETURN_TYPESTATE(condition)
+ #define SEASTAR_SET_TYPESTATE(condition)
+ #define SEASTAR_PARAM_TYPESTATE(condition)
+#endif
--
2.39.2

[tcha...@gmail.com]

hello? ping for reviews.

i think it's a very nice facility and it helped us find some issues in scylladb already. it will also benefit other Seastar projects as well. having it in Seastar will prevent us from stepping into similar issues in future.

cheers,

[Avi Kivity]

What about this?

Let's have just SEASTAR_CLANG_ATTRIBUTE(attr) instead of indirection through these. So if I see SEASTAR_CLANG_ATTRIBUTE([[clang::consumable(unconsumed)]]) I can google it without having to unwrap the macros.

--

2.39.2

[Avi Kivity]

Even better, there is "-Wno-attributes=clang::" which will cause gcc to ignore clang attributes.

[tcha...@gmail.com]

hi Raphael, are you still interested in continuing working on this change? for instance, to address the comments from Avi? if not, may i take the torch from you and send an updated version? i'd like to have this change in Seastar, especially because you've already done the heavy lifting, and IMHO, the patch is quite close.

[Raphael S. Carvalho]

On Thu, Jun 15, 2023 at 11:51 PM tcha...@gmail.com <tcha...@gmail.com> wrote:

hi Raphael, are you still interested in continuing working on this change? for instance, to address the comments from Avi? if not, may i take the torch from you and send an updated version? i'd like to have this change in Seastar, especially because you've already done the heavy lifting, and IMHO, the patch is quite close.

Hi my friend. I can pick it up. Will send v2 soon.

 

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/4be6ab5d-097f-423e-9aa6-b1af6d8eaf19n%40googlegroups.com.

[tcha...@gmail.com]

On Tuesday, June 20, 2023 at 12:18:45 AM UTC+8 raph...@scylladb.com wrote:

On Thu, Jun 15, 2023 at 11:51 PM tcha...@gmail.com <tcha...@gmail.com> wrote:

hi Raphael, are you still interested in continuing working on this change? for instance, to address the comments from Avi? if not, may i take the torch from you and send an updated version? i'd like to have this change in Seastar, especially because you've already done the heavy lifting, and IMHO, the patch is quite close.

Hi my friend. I can pick it up. Will send v2 soon.

Thank you, Raphael!

 

[Kefu Chai]

Hi Raphael, i took the liberty and replicated your change to https://github.com/scylladb/seastar/pull/2029. if you still plan to send the v2 over the discussion group soon, i will close it. or we can probably cooperate on the github using that pull request?

what do you think?

 

Thank you, Raphael!

 

[Raphael S. Carvalho]

Hi Kefu, I dropped the ball on this one. Thanks a lot for picking it up. We can definitely cooperate there. 

 

 

Thank you, Raphael!

 

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/d3e75ad7-6c67-4e7b-8f75-51b7f343292bn%40googlegroups.com.