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

LoadTableHandlers can cause std::terminate #13303

Open
Ignition opened this issue Jan 16, 2025 · 3 comments
Open

LoadTableHandlers can cause std::terminate #13303

Ignition opened this issue Jan 16, 2025 · 3 comments

Comments

@Ignition
Copy link

#2  0x000055a53cb70c3a in __cxxabiv1::__terminate(void (*)()) ()
#3  0x000055a53cb70ca5 in std::terminate() ()
#4  0x000055a53c768a81 in std::vector<std::thread, std::allocator<std::thread> >::~vector() ()
#5  0x000055a53c8f3d3a in rocksdb::VersionBuilder::Rep::LoadTableHandlers(rocksdb::InternalStats*, int, bool, bool, std::shared_ptr<rocksdb::SliceTransform const> const&, unsigned long) ()

Relevant code

std::vector<port::Thread> threads;
for (int i = 1; i < max_threads; i++) {
  threads.emplace_back(load_handlers_func);
}
load_handlers_func();
for (auto& t : threads) {
  t.join();
}

As far as I can tell either an exception came from emplace_back or from load_handlers_func. But in either case it caused the std::thread's destructor to have an issue because it wasn't joined yet and hence std::terminate was called.

std::thread::~thread If *this has an associated thread (joinable() == true), std::terminate() is called.

@Ignition
Copy link
Author

Also unrelated observation, maybe worth adding if (files_meta.empty()) return Status::OK(); before making any worker threads.

Ignition added a commit to memgraph/memgraph that referenced this issue Jan 17, 2025
Ignition added a commit to memgraph/memgraph that referenced this issue Jan 17, 2025
Ignition added a commit to memgraph/memgraph that referenced this issue Jan 22, 2025
@jowlyzhang
Copy link
Contributor

RocksDB doesn't throw Exceptions or handle exceptions, see https://github.com/facebook/rocksdb/wiki/RocksDB-FAQ#failure-handling (mostly). Most customizable interfaces' documentation will have notes like this:

// * Exceptions MUST NOT propagate out of overridden functions into RocksDB,
// because RocksDB is not exception-safe. This could cause undefined behavior
// including data loss, unreported corruption, deadlocks, and more.

@Ignition
Copy link
Author

Ignition commented Feb 17, 2025

@jowlyzhang this is not via customizable interface.

This is purely an internal correctness detail within RocksDB.

This is just standard C++. You are using std::vector::emplace_back without any custom allocator, hence it calls operator new which can throw at least std::bad_alloc (additional exceptions are allowed to be throw cause operator new can be replaced). And then it tries to destroy std::vector and hence it destroys its std::threads which will std::terminate because it wasn't joined.

Unintentionally calling std::terminate and killing the whole program should be considered a critical bug.

@Ignition Ignition changed the title LoadTableHandlers not exception safe LoadTableHandlers can cause std::terminate Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants