-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Catch use-after-free bugs in usage of shared ptrs using Clang's attributes #2029
base: master
Are you sure you want to change the base?
Conversation
this change is copied from https://groups.google.com/g/seastar-dev/c/hKxNokDaHMY/m/t92rRO4dAgAJ , in hope that it can facilitate the review process. v2:
|
@@ -608,12 +635,15 @@ public: | |||
explicit operator bool() const noexcept { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quote from Avi's review comment at https://groups.google.com/g/seastar-dev/c/hKxNokDaHMY/m/etJQnAhJAQAJ
What about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method only accesses _p
, and does not de-reference or mutate it, so it does not depends on any preconditions imposed on the consumed/unconsumed state of the instance, neither does it set the consumed/unconsumed state, so no need to annotate it.
766380e
to
81c4911
Compare
@avikivity @raphaelsc hi Avi and Raphael, could you help review this change? |
FWIW, i compiled scylla master HEAD with this change, no |
81c4911
to
94f6744
Compare
v3:
|
@raphaelsc hi Raph, thank you for your review! could you help re-review this change? |
@scylladb/seastar-maint hello maintainers, could you help review this change? |
ping for merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first time I'm seeing this PR (I believe @avikivity reviewed it in the past), so I'm unfamiliar with the clang feature. Looks like a nice feature, but if I understand correctly, it doesn't actually do anything if you don't explicitly add a -Wconsumed to your build? If that's the case, don't you want to add -Wconsumed by default? If not, why not - and do we need to document this anywhere so users will find it? Do you plan to enable it by default in the ScyllaDB project?
I left a small question that probably reflects more on my ignorance than on the correctness of this patch.
include/seastar/core/shared_ptr.hh
Outdated
@@ -738,35 +766,35 @@ SEASTAR_MODULE_EXPORT_BEGIN | |||
template <typename T, typename U> | |||
inline | |||
bool | |||
operator==(const shared_ptr<T>& x, const shared_ptr<U>& y) { | |||
operator==(const shared_ptr<T>& x [[clang::param_typestate(unconsumed)]], const shared_ptr<U>& y [[clang::param_typestate(unconsumed)]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these annotations needed or helpful? This function calls x.get() on x, so should already fail if x is consumed. Or maybe the "unknown" case is the difference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think so. dropped in the latest revision. actually. i don't think we should enforce any expectation here. as any shared_ptr
can be compared with any shared_ptr
. and neither should we enforce any expectation when calling get()
, as shared_ptr::get()
should behave no matter it is consumed or not.
include/seastar/core/shared_ptr.hh
Outdated
T* operator->() const noexcept { | ||
return _p; | ||
} | ||
[[clang::callable_when("unconsumed", "unknown")]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is "unknown" ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is based on practical reasons. the state of a consumable object is based on annotations, but not all the code paths are annotated. in the code paths where the annotation is missing, and compiler is not able to deduce the state of the object, it's state would be "unknown", but we should not fail the build just because the compiler is not able to figure out the runtime behavior without enough annotations. actually, we have lots of them in scylla.
to make the -Wconsume
useful before annotating all of them, adding "unknown" is a good compromise. probably is a must.
thank you Nadav, this makes a lot of sense! we should both enable and expose this compile option as a public cflag exposed to the parent project. class [[clang::consumable(unconsumed)]] CleverObject {
public:
[[clang::return_typestate(unconsumed)]]
CleverObject() {}
CleverObject(CleverObject&& other) { other.invalidate(); }
[[clang::callable_when(unconsumed)]]
void do_something() { assert(m_valid); }
private:
[[clang::set_typestate(consumed)]]
void invalidate() { m_valid = false; }
bool m_valid { true };
};
SEASTAR_THREAD_TEST_CASE(test_foo) {
CleverObject object;
auto other = std::move(object);
object.do_something();
} and the build failed as expected
|
…med attributes 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 <raphaelsc@scylladb.com>
94f6744
to
a6c3f06
Compare
clang-17 is not able to compile with
while clang-18 is able to build the tree. |
even with clang-19 (c761b4a5e4cc003a2c850898e1dc67d2637cfb0c), i still have build failures like
i am afraid that this is a show stopper. |
i will revisit this change later on. before that, let's mark this change "draft". |
Clang provides consumed annotations, which are very useful for statically catching use-after-free bugs in specific classes.
That's done by:
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.