Skip to content

Fix MSAN use-after-free in Lua HeaderMapIterator destruction #43204

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.

Sign up for GitHub

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

Open
ashutosh321607 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix MSAN use-after-free in Lua HeaderMapIterator destruction #43204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions source/extensions/filters/common/lua/lua.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,13 @@ template <typename T> class LuaDeathRef : public LuaRef<T> {
markDead();
LuaRef<T>::reset();
}

/**
* Resets the reference without calling markDead().
* Use this only when the referenced object is already being destroyed (e.g. inside its
* destructor) to avoid undefined behavior.
*/
void resetWithoutMarkDead() { LuaRef<T>::reset(); }
};

/**
Expand Down
22 changes: 18 additions & 4 deletions source/extensions/filters/http/lua/wrappers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,26 @@ namespace Extensions {
namespace HttpFilters {
namespace Lua {

HeaderMapIterator::HeaderMapIterator(HeaderMapWrapper& parent) : parent_(parent) {
entries_.reserve(parent_.headers_.size());
parent_.headers_.iterate(
HeaderMapIterator::HeaderMapIterator(HeaderMapWrapper& parent) : parent_(&parent) {
entries_.reserve(parent_->headers_.size());
parent_->headers_.iterate(
[this](const Envoy::Http::HeaderEntry& header) -> Envoy::Http::HeaderMap::Iterate {
entries_.push_back(&header);
return Envoy::Http::HeaderMap::Iterate::Continue;
});
}

HeaderMapIterator::~HeaderMapIterator() {
if (parent_ != nullptr) {
parent_->onIteratorDestroyed(this);
}
}

int HeaderMapIterator::luaPairsIterator(lua_State* state) {
if (current_ == entries_.size()) {
parent_.iterator_.reset();
if (parent_ != nullptr) {
parent_->iterator_.reset();
}
return 0;
} else {
const absl::string_view key_view(entries_[current_]->key().getStringView());
Expand Down Expand Up @@ -127,6 +135,12 @@ void HeaderMapWrapper::checkModifiable(lua_State* state) {
}
}

void HeaderMapWrapper::onIteratorDestroyed(HeaderMapIterator* it) {
if (iterator_.get() == it) {
iterator_.resetWithoutMarkDead();
}
}

int HeaderMapWrapper::luaSetHttp1ReasonPhrase(lua_State* state) {
checkModifiable(state);

Expand Down
6 changes: 5 additions & 1 deletion source/extensions/filters/http/lua/wrappers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ class HeaderMapWrapper;
class HeaderMapIterator : public Filters::Common::Lua::BaseLuaObject<HeaderMapIterator> {
public:
HeaderMapIterator(HeaderMapWrapper& parent);
~HeaderMapIterator() override;

static ExportedFunctions exportedFunctions() { return {}; }

DECLARE_LUA_CLOSURE(HeaderMapIterator, luaPairsIterator);

void onMarkDead() override { parent_ = nullptr; }

private:
HeaderMapWrapper& parent_;
HeaderMapWrapper* parent_;
std::vector<const Http::HeaderEntry*> entries_;
uint64_t current_{};
};
Expand Down Expand Up @@ -114,6 +117,7 @@ class HeaderMapWrapper : public Filters::Common::Lua::BaseLuaObject<HeaderMapWra
DECLARE_LUA_FUNCTION(HeaderMapWrapper, luaSetHttp1ReasonPhrase);

void checkModifiable(lua_State* state);
void onIteratorDestroyed(HeaderMapIterator* it);

// Envoy::Lua::BaseLuaObject
void onMarkDead() override {
Expand Down
Loading