Fix VFS use-after-move crash by using stable handles for mjVFS
Root cause: resources and default mount stored pointers tied to the public mjVFS object address. If mjVFS was moved after initialization, resource close/read paths could dereference stale pointers and crash.\n\nThis change stores a stable internal mjVFS handle inside VFS and points resources/default mount to that stable handle. It also tracks the current public owner pointer and rebinds it in Upcast so lifecycle operations (including self-destruct cleanup) target the current mjVFS address after moves.\n\nAdds regression test UserVfsTest.MoveVfsAfterOpenResource, which opens a resource, moves mjVFS, then reads/closes/deletes successfully.
This commit is contained in:
+19
-6
@@ -105,7 +105,9 @@ std::string StripPathAndLower(std::string path) {
|
||||
|
||||
namespace mujoco::user {
|
||||
|
||||
VFS::VFS(mjVFS* vfs) : self_(vfs) {
|
||||
VFS::VFS(mjVFS* vfs) : owner_(vfs) {
|
||||
stable_vfs_.impl_ = this;
|
||||
|
||||
mjp_defaultResourceProvider(&default_provider_);
|
||||
default_provider_.open = [](mjResource* res) {
|
||||
return OpenFile(res->name, res);
|
||||
@@ -121,7 +123,7 @@ VFS::VFS(mjVFS* vfs) : self_(vfs) {
|
||||
};
|
||||
|
||||
default_provider_.prefix = nullptr;
|
||||
default_mount_.vfs = self_;
|
||||
default_mount_.vfs = &stable_vfs_;
|
||||
default_mount_.provider = &default_provider_;
|
||||
default_mount_.data = nullptr;
|
||||
default_mount_.name = nullptr;
|
||||
@@ -245,7 +247,7 @@ int VFS::Read(mjResource* resource, const void** buffer) {
|
||||
VFS::ResourcePtr VFS::CreateResource(std::string_view name,
|
||||
const mjpResourceProvider* provider) {
|
||||
mjResource* res = new mjResource();
|
||||
res->vfs = self_;
|
||||
res->vfs = &stable_vfs_;
|
||||
res->provider = provider;
|
||||
res->data = nullptr;
|
||||
res->name = new char[name.size() + 1];
|
||||
@@ -312,20 +314,31 @@ mjResource* VFS::FindMount(const std::string& fullpath) {
|
||||
|
||||
void VFS::MaybeSelfDestruct() {
|
||||
if (destructor_) {
|
||||
destructor_(self_);
|
||||
destructor_(owner_);
|
||||
}
|
||||
}
|
||||
|
||||
void VFS::Bind(mjVFS* vfs) {
|
||||
owner_ = vfs;
|
||||
}
|
||||
|
||||
void VFS::SetToSelfDestruct(std::function<void(mjVFS*)> destructor) {
|
||||
destructor_ = std::move(destructor);
|
||||
}
|
||||
|
||||
VFS* VFS::Upcast(mjVFS* vfs) {
|
||||
return vfs ? static_cast<VFS*>(vfs->impl_) : nullptr;
|
||||
if (!vfs) {
|
||||
return nullptr;
|
||||
}
|
||||
VFS* impl = static_cast<VFS*>(vfs->impl_);
|
||||
if (impl) {
|
||||
impl->Bind(vfs);
|
||||
}
|
||||
return impl;
|
||||
}
|
||||
|
||||
const VFS* VFS::Upcast(const mjVFS* vfs) {
|
||||
return vfs ? static_cast<const VFS*>(vfs->impl_) : nullptr;
|
||||
return Upcast(const_cast<mjVFS*>(vfs));
|
||||
}
|
||||
|
||||
} // namespace mujoco::user
|
||||
|
||||
+6
-1
@@ -117,7 +117,12 @@ class VFS {
|
||||
// that `this` will be invalidated after this call.
|
||||
void MaybeSelfDestruct();
|
||||
|
||||
mjVFS* self_;
|
||||
// Rebinds the current public mjVFS pointer to this implementation.
|
||||
// This supports scenarios where the public mjVFS struct is moved.
|
||||
void Bind(mjVFS* vfs);
|
||||
|
||||
mjVFS stable_vfs_;
|
||||
mjVFS* owner_;
|
||||
std::mutex mutex_; // Protects open_resources_ and mounts_.
|
||||
std::unordered_map<mjResource*, ResourcePtr> open_resources_;
|
||||
std::unordered_map<std::string, ResourcePtr> mounts_;
|
||||
|
||||
@@ -15,6 +15,7 @@
|
||||
#include <array>
|
||||
#include <cstdio>
|
||||
#include <string>
|
||||
#include <utility>
|
||||
|
||||
#include <gmock/gmock.h>
|
||||
#include <gtest/gtest.h>
|
||||
@@ -286,6 +287,29 @@ TEST_F(UserVfsTest, Timestamps) {
|
||||
mj_deleteVFS(&vfs);
|
||||
}
|
||||
|
||||
TEST_F(UserVfsTest, MoveVfsAfterOpenResource) {
|
||||
mjVFS vfs;
|
||||
mj_defaultVFS(&vfs);
|
||||
|
||||
std::string buffer = "<mujoco/>";
|
||||
mj_addBufferVFS(&vfs, "model", static_cast<const void*>(buffer.c_str()),
|
||||
buffer.size());
|
||||
|
||||
mjResource* resource = mju_openResource("", "model", &vfs, nullptr, 0);
|
||||
ASSERT_THAT(resource, NotNull());
|
||||
|
||||
// Move the public mjVFS object after resources have been opened.
|
||||
mjVFS moved = std::move(vfs);
|
||||
|
||||
const void* out = nullptr;
|
||||
const int size = mju_readResource(resource, &out);
|
||||
EXPECT_GT(size, 0);
|
||||
EXPECT_THAT(out, NotNull());
|
||||
|
||||
mju_closeResource(resource);
|
||||
mj_deleteVFS(&moved);
|
||||
}
|
||||
|
||||
TEST_F(UserVfsTest, MountUnmount) {
|
||||
int test = 0;
|
||||
int expect = 0;
|
||||
|
||||
Reference in New Issue
Block a user