Skip to content

Commit

Permalink
Fix abort() call on appsec helper unload
Browse files Browse the repository at this point in the history
runner was living too long (until shared library unload) due to a static
shared pointer used for RC notifications. Plus, the destructor would
have a call to shared_for_this(), which would try to revive the shared
pointer being destroyed, which raise an exception due to there being no
shared pointer anymore. We would catch this and abort().

Instead, destroy the runner earlier (when its own thread finishes).
Reset the static shared pointer just before that.
  • Loading branch information
cataphract committed Oct 18, 2024
1 parent f5c5729 commit 38e3959
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 6 deletions.
5 changes: 4 additions & 1 deletion appsec/src/helper/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ int appsec_helper_main_impl()

runner->run();

runner->unregister_for_rc_notifications();

finished.store(true, std::memory_order_release);
}};
thread_id = thr.native_handle();
Expand Down Expand Up @@ -156,7 +158,8 @@ appsec_helper_shutdown() noexcept
pthread_kill(thread_id, SIGUSR1);

// wait up to 1 second for the runner to finish
auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds{1};
auto deadline =
std::chrono::steady_clock::now() + std::chrono::seconds{1};
while (true) {
if (finished.load(std::memory_order_acquire)) {
SPDLOG_INFO("AppSec helper finished");
Expand Down
12 changes: 8 additions & 4 deletions appsec/src/helper/runner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ void runner::register_for_rc_notifications()
std::atomic_load(&RUNNER_FOR_NOTIFICATIONS);
if (!runner) {
// NOLINTNEXTLINE(bugprone-lambda-function-name)
SPDLOG_ERROR("No runner to notify of remote config updates");
SPDLOG_WARN("No runner to notify of remote config updates");
ddog_remote_config_path_free(path);
return;
}
Expand All @@ -136,15 +136,19 @@ void runner::register_for_rc_notifications()
});
}

runner::~runner() noexcept
void runner::unregister_for_rc_notifications()
{
SPDLOG_INFO("Unregister runner for RC update callback");
try {
std::shared_ptr<runner> expected = shared_from_this();
std::atomic_compare_exchange_strong(&RUNNER_FOR_NOTIFICATIONS,
&expected, std::shared_ptr<runner>(nullptr));
} catch (...) {
// can only happened if there is no shared_ptr for the runner
// in this case a std::bad_weak_ptr is thrown
// can only happen if there is no shared_ptr for the runner
// in this case a std::bad_weak_ptr is thrown.
// But we only expose runner through a shared pointer, so this would
// require extraordinary actions to destroy the shared pointer but not
// the object.
std::abort();
}
}
Expand Down
4 changes: 3 additions & 1 deletion appsec/src/helper/runner.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ class runner : public std::enable_shared_from_this<runner> {
runner &operator=(const runner &) = delete;
runner(runner &&) = delete;
runner &operator=(runner &&) = delete;
~runner() noexcept;
~runner() = default;

static void resolve_symbols();

void run() noexcept(false);

void register_for_rc_notifications();

void unregister_for_rc_notifications();

[[nodiscard]] bool interrupted() const
{
return interrupted_.load(std::memory_order_acquire);
Expand Down

0 comments on commit 38e3959

Please sign in to comment.