Catch use-after-free bugs in usage of shared ptrs using Clang's attributes #2029
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.
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.
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.
tchaikov
changed the title
tchaikov
force-pushed
the
catch-use-after-free-clang-attr
branch
4 times, most recently
from
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 |
tchaikov
force-pushed
the
catch-use-after-free-clang-attr
branch
from
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.
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.
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.
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.
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>
tchaikov
force-pushed
the
catch-use-after-free-clang-attr
branch
from
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.