Skip to content

Commit

Permalink
Bug#36081753 use-after-free in TcpPortPool
Browse files Browse the repository at this point in the history
When built with ASAN, a use-after-free is reported for the TcpPortPool.

AddressSanitizer: heap-use-after-free on address 0x60200019f190 at pc
0x00000076a18d bp 0x7fff51e7d1d0 sp 0x7fff51e7d1c0

    #4 0x770b73 in UniqueId::ProcessUniqueIds::erase(unsigned int)
       ../router/tests/helpers/tcp_port_pool.h:112
    #5 0x770c48 in UniqueId::~UniqueId()
       ../router/tests/helpers/tcp_port_pool.cc:234
    ...
    #12 0x82faa3 in testing::UnitTest::~UnitTest()
	../extra/googletest/googletest-release-1.12.0/googletest/src/gtest.cc:5496
    percona#13 0x7f5fe085ace8 in __run_exit_handlers (/lib64/libc.so.6+0x39ce8)

0x60200019f190 is located 0 bytes inside of 16-byte region
[0x60200019f190,0x60200019f1a0)
freed by thread T0 here:
    #0 0x7f5fe3cbd10f in operator delete(void*, unsigned long)
       (/lib64/libasan.so.6+0xb710f)
    #1 0x7f5fe085ace8 in __run_exit_handlers (/lib64/libc.so.6+0x39ce8)

Background
==========

__run_exit_handlers destroys "static" and "global" variables in reverse
order of their creation.

googletest's unit-tests are a static, and the TcpPortPool also has
ProcessUniqueId's which contains the process-wide unique-ids.

At construct: unittest -> tcp-port-pool -> proces-unique-ids
At destruct : process-unique-ids -> tcp-port-pool -> 💥

The use-after-free happens as the process-unique-ids static is
destructed before the tcp-port-pool which tries to its Ids from the
process-unique-ids.

Change
======

- extend the lifetime of the process-unique-ids to after the last use of
  the tcp-port-pool via a std::shared_ptr<>

Change-Id: I75b8b781e1d240f18ca72f2c86182639a7699f06
  • Loading branch information
weigon committed Dec 6, 2023
1 parent 3a492fe commit 3646343
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 9 deletions.
33 changes: 26 additions & 7 deletions router/tests/helpers/tcp_port_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@
#endif

#include <cstring>
#include <memory> // shared_ptr
#include <system_error>
#include <utility>
#include <utility> // move

#include "mysql/harness/filesystem.h" // Path
#include "mysql/harness/stdx/expected.h"
Expand Down Expand Up @@ -199,7 +200,8 @@ std::string UniqueId::get_lock_file_dir() {
#endif
}

UniqueId::UniqueId(value_type start_from, value_type range) {
UniqueId::UniqueId(value_type start_from, value_type range)
: proc_ids_(process_unique_ids()) {
const std::string lock_file_dir = get_lock_file_dir();
mysql_harness::mkdir(lock_file_dir, 0777);
#ifndef _WIN32
Expand All @@ -209,7 +211,7 @@ UniqueId::UniqueId(value_type start_from, value_type range) {
#endif

for (auto i = start_from; i < start_from + range; i++) {
if (process_unique_ids().contains(i)) continue;
if (proc_ids_->contains(i)) continue;

Path lock_file_path(lock_file_dir);
lock_file_path.append(std::to_string(i));
Expand All @@ -218,7 +220,7 @@ UniqueId::UniqueId(value_type start_from, value_type range) {
if (lock_res) {
id_ = i;

process_unique_ids().insert(i);
proc_ids_->insert(i);

// obtained the lock, we are good to go
return;
Expand All @@ -231,12 +233,29 @@ UniqueId::UniqueId(value_type start_from, value_type range) {
UniqueId::~UniqueId() {
// release the process unique-id if we own one.
if (id_) {
process_unique_ids().erase(*id_);
proc_ids_->erase(*id_);
}
}

UniqueId::ProcessUniqueIds &UniqueId::process_unique_ids() {
static ProcessUniqueIds ids;
// process-wide unique-ids
//
// is a "static shared_ptr<>" instead of a "static" as the TcpPort may be part
// of a "static" too.
//
// It would create:
// 1. (static) TcpPortPool
// 2. static ProcessUniqueIds
//
// ... and then at destruct in reverse order:
// * ProcessUniqueIds
// * TcpPortPool ... but the TcpPortPool would try to remove itself from the
// ProcessUniqueIds
//
//
//
std::shared_ptr<UniqueId::ProcessUniqueIds> UniqueId::process_unique_ids() {
static std::shared_ptr<UniqueId::ProcessUniqueIds> ids =
std::make_shared<UniqueId::ProcessUniqueIds>();

return ids;
}
Expand Down
9 changes: 7 additions & 2 deletions router/tests/helpers/tcp_port_pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#define _TCP_PORT_POOL_H_

#include <cstdint>
#include <memory>
#include <optional>
#include <string>
#include <system_error>
Expand Down Expand Up @@ -120,10 +121,12 @@ class UniqueId {
UniqueId(const UniqueId &) = delete;
UniqueId &operator=(const UniqueId &) = delete;
UniqueId(UniqueId &&other) noexcept
: id_(std::exchange(other.id_, {})),
: proc_ids_(std::move(other.proc_ids_)),
id_(std::exchange(other.id_, {})),
lock_file_fd_(std::exchange(other.lock_file_fd_, {})) {}

UniqueId &operator=(UniqueId &&other) noexcept {
proc_ids_ = std::move(other.proc_ids_);
id_ = std::exchange(other.id_, {});
lock_file_fd_ = std::exchange(other.lock_file_fd_, {});

Expand All @@ -137,7 +140,9 @@ class UniqueId {
private:
stdx::expected<void, std::error_code> lock_file(const std::string &file_name);

static ProcessUniqueIds &process_unique_ids();
static std::shared_ptr<UniqueId::ProcessUniqueIds> process_unique_ids();

std::shared_ptr<UniqueId::ProcessUniqueIds> proc_ids_;

static std::string get_lock_file_dir();

Expand Down

0 comments on commit 3646343

Please sign in to comment.