Skip to content

Commit

Permalink
Fix crash on exit with sentry enabled (due to openssl destroyed befor…
Browse files Browse the repository at this point in the history
…e sentry)

The problem is tha openssl registers OPENSSL_cleanup() as atexit
handler, which called before destroying of SentryWriter, so to avoid
this problem, let's destroy it explicitly.

<details>

<summary>stack trace example</summary>

    Thread 2 (Thread 0x7ffff54006c0 (LWP 24847) "clickhouse-serv"):
    0  ___pthread_rwlock_rdlock (rwlock=0x0) at pthread_rwlock_rdlock.c:26
    1  0x00000000164c18a9 in CRYPTO_THREAD_read_lock (lock=0x0) at threads_pthread.c:93
    2  0x000000001642e6b9 in int_err_get_item (d=0x7ffff53f74e0) at err.c:192
    ...
    7  ossl_connect_common (cf=0x7ffff7812c80, data=0x7ffff70a4c00, nonblocking=bool_true, done=0x7ffff53f834c) at openssl.c:4486
    ...
    17 curl_easy_perform (data=data@entry=0x7ffff70a4c00) at easy.c:787
    18 0x000000000b4c3854 in sentry__curl_send_task (_envelope=<optimized out>, _state=0x7ffff7074300) at sentry_transport_curl.c:225
    19 0x000000000b4ba880 in worker_thread (data=0x7ffff70e5500) at sentry_sync.c:262

    Thread 1 (Thread 0x7ffff7cb2c80 (LWP 24842) "clickhouse-serv"):
    5  0x000000000b4bb0e2 in sentry__cond_wait_timeout (cv=0x7ffff70e5540, mutex=0x7ffff70e5570, msecs=250) at sentry_sync.h:332
    6  sentry__bgworker_shutdown (bgw=0x7ffff70e5500, timeout=2000) at sentry_sync.c:412
    7  0x000000000b4b3e95 in sentry_close () at sentry_core.c:238
    8  0x000000000b4a5f1f in SentryWriter::~SentryWriter (this=0x7ffff71a1240) at SentryWriter.cpp:147
    9  std::__1::default_delete<SentryWriter>::operator()[abi:v15000](SentryWriter*) const (this=0x7ffff70e5568, __ptr=0x7ffff71a1240) at unique_ptr.h:48
    10 std::__1::unique_ptr<SentryWriter, std::__1::default_delete<SentryWriter> >::reset[abi:v15000](SentryWriter*) (this=0x7ffff70e5568, __p=0x0) at unique_ptr.h:305
    11 std::__1::unique_ptr<SentryWriter, std::__1::default_delete<SentryWriter> >::~unique_ptr[abi:v15000]() (this=0x7ffff70e5568) at unique_ptr.h:259
    12 0x00007ffff7de62e6 in __run_exit_handlers (status=0, listp=<optimized out>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
    13 0x00007ffff7de642e in __GI_exit (status=<optimized out>) at exit.c:138
    14 0x00007ffff7dccd51 in __libc_start_call_main (main=main@entry=0x6111c20 <main(int, char**)>, argc=argc@entry=13, argv=argv@entry=0x7fffffffb718) at libc_start_call_main.h:74
    15 0x00007ffff7dcce0c in __libc_start_main_impl (main=0x6111c20 <main(int, char**)>, argc=13, argv=0x7fffffffb718, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffb708) at libc-start.c:360

    (gdb) p req.body
    $7 = 0x7ffff7816000 "{\"dsn\":\"...\"}\n{\"type\":\"session\",\"length\":190}\n{\"init\":true,\"sid\":\"...\",\"status\":\"exited\",\"errors\":0,\"started\":\"2024-05-08T20:29:23.253Z\",\"duration\":17.213,\"attrs\":{\"release\":\"24.5\",\"environment\":\"test\"}}"

</details>

P.S. Likely started happens after conversion to OpenSSL (ClickHouse#59870).

Signed-off-by: Azat Khuzhin <[email protected]>
  • Loading branch information
azat committed May 9, 2024
1 parent dc7f515 commit 27b4165
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/Daemon/BaseDaemon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,8 @@ BaseDaemon::~BaseDaemon()
}

signal_pipe.close();

SentryWriter::resetInstance();
}


Expand Down
5 changes: 5 additions & 0 deletions src/Daemon/SentryWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ SentryWriter * SentryWriter::getInstance()
{
return SentryWriter::instance.get();
}
void SentryWriter::resetInstance()
{
SentryWriter::instance.reset();
}

SentryWriter::SentryWriter(Poco::Util::LayeredConfiguration & config)
{
Expand Down Expand Up @@ -254,6 +258,7 @@ void SentryWriter::sendError(Type type, int sig_or_error, const std::string & er

void SentryWriter::initializeInstance(Poco::Util::LayeredConfiguration &) {}
SentryWriter * SentryWriter::getInstance() { return nullptr; }
void SentryWriter::resetInstance() {}

SentryWriter::SentryWriter(Poco::Util::LayeredConfiguration &) {}
SentryWriter::~SentryWriter() = default;
Expand Down
7 changes: 7 additions & 0 deletions src/Daemon/SentryWriter.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,16 @@ class SentryWriter

/// Initialize static SentryWriter instance
static void initializeInstance(Poco::Util::LayeredConfiguration & config);

/// @return nullptr if initializeInstance() was not called (i.e. for non-server) or SentryWriter object
static SentryWriter * getInstance();

/// SentryWriter static instance should be reset explicitly to avoid
/// possible use-after-free, since it may use some global objects (i.e.
/// OpenSSL), while sending final statistics
/// (SENTRY_SESSION_STATUS_EXITED).
static void resetInstance();

void onSignal(
int sig,
const std::string & error_message,
Expand Down

0 comments on commit 27b4165

Please sign in to comment.