Skip to content
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

vk: Use-after-free coverage #8273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions filament/backend/src/vulkan/memory/Resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ struct Resource {
: resManager(nullptr),
id(HandleBase::nullid),
mCount(0),
restype(ResourceType::UNDEFINED_TYPE) {}
restype(ResourceType::UNDEFINED_TYPE),
mHandleConsideredDestroyed(false) {}

private:
inline void inc() noexcept {
Expand All @@ -80,6 +81,16 @@ struct Resource {
}
}

// To be able to detect use-after-free, we need a bit to signify if the handle should be
// consider destroyed (from Filament's perspective).
inline void setHandleConsiderDestroyed() noexcept {
mHandleConsideredDestroyed = true;
}

inline bool isHandleConsideredDestroyed() const {
return mHandleConsideredDestroyed;
}

template <typename T>
inline void init(HandleId id, ResourceManager* resManager) {
this->id = id;
Expand All @@ -92,7 +103,9 @@ struct Resource {
ResourceManager* resManager; // 8
HandleId id; // 4
uint32_t mCount : 24;
ResourceType restype : 6; // restype + mCount is 4 bytes.
ResourceType restype : 7;
bool mHandleConsideredDestroyed : 1; // restype + mCount + mHandleConsideredDestroyed
// is 4 bytes.

friend class ResourceManager;

Expand All @@ -105,7 +118,8 @@ struct ThreadSafeResource {
: resManager(nullptr),
id(HandleBase::nullid),
mCount(0),
restype(ResourceType::UNDEFINED_TYPE) {}
restype(ResourceType::UNDEFINED_TYPE),
mHandleConsideredDestroyed(false) {}

private:
inline void inc() noexcept {
Expand All @@ -118,6 +132,16 @@ struct ThreadSafeResource {
}
}

// To be able to detect use-after-free, we need a bit to signify if the handle should be
// consider destroyed (from Filament's perspective).
inline void setHandleConsiderDestroyed() noexcept {
mHandleConsideredDestroyed = true;
}

inline bool isHandleConsideredDestroyed() const {
return mHandleConsideredDestroyed;
}

template <typename T>
inline void init(HandleId id, ResourceManager* resManager) {
this->id = id;
Expand All @@ -130,7 +154,8 @@ struct ThreadSafeResource {
ResourceManager* resManager; // 8
HandleId id; // 4
std::atomic<uint32_t> mCount; // 4
ResourceType restype; // 1
ResourceType restype : 7;
bool mHandleConsideredDestroyed : 1; // restype + mHandleConsideredDestroyed is 1 byte

friend class ResourceManager;

Expand Down
8 changes: 8 additions & 0 deletions filament/backend/src/vulkan/memory/ResourcePointer.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "vulkan/memory/ResourceManager.h"

#include <backend/Handle.h>
#include <utils/compiler.h>

#include <utility>

Expand Down Expand Up @@ -59,6 +60,11 @@ struct resource_ptr {
static enabled_resource_ptr<B> cast(ResourceManager* resManager,
Handle<B> const& handle) noexcept {
D* ptr = resManager->handle_cast<D*, B>(handle);
if (UTILS_UNLIKELY(ptr->isHandleConsideredDestroyed())) {
FILAMENT_CHECK_PRECONDITION(!ptr->isHandleConsideredDestroyed())
<< "Handle id=" << ptr->id << " (" << getTypeStr(ptr->restype)
<< ") is being used after it has been freed";
}
Comment on lines +63 to +67
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need the if here, FILAMENT_CHECK_PRECONDITION already does all the likely/unlikely shenanigans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to avoid hitting the switch in getTypeStr unless necessary, hence the roundabout logic here.
Do you think it's worth it or not? (or maybe it just going to be speculatively executed anyways?).

return {ptr};
}

Expand Down Expand Up @@ -156,6 +162,8 @@ struct resource_ptr {
// only be used from VulkanDriver.
inline void dec() {
assert_invariant(mRef);
assert_invariant(!mRef->isHandleConsiderDestroyed());
mRef->setHandleConsiderDestroyed();
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we at least assert_invariant() here that the handle is not already in the considered destroyed state? (I think this should never happen right?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mRef->dec();
}

Expand Down
Loading