Attempt to fix Android app crashes with C++ library (#455)

TomTasche · claude · andiwand · web-flow · commit 71cb00297a90 · 2025-11-30T15:06:07.000+01:00
* fix: prevent SIGSEGV crash in httplib HTTP server on Android

This fixes a crash that was affecting ~950 users with the following
root causes:

1. Chunked transfer encoding issues: Replace ContentProviderWithoutLength
   with buffered content using set_content(). The streaming content
   provider could crash when:
   - Client disconnects during transfer
   - Exceptions thrown during content generation
   - Server stopped while requests in-flight

2. Unhandled exceptions: Add try-catch blocks around request handling
   to gracefully handle exceptions instead of letting them propagate
   into httplib's internals where they caused SIGSEGV.

3. Race condition in stop(): Reorder operations to call m_server.stop()
   before clear() so that in-flight requests complete before resources
   are invalidated.

The crash was reported as SIGSEGV in
httplib::Server::write_response_core on various Android devices and
versions (Android 10-16, Samsung, Motorola devices).

* fix: prevent crash in httplib process_request during shutdown

Additional fixes for Android crashes in httplib HTTP server:

1. Add destructor to Impl class: Ensures the server is properly stopped
   before the Impl object is destroyed. This prevents worker threads
   from accessing freed memory (use-after-free crash).

2. Add atomic stopping flag: Handlers check this flag and return 503
   Service Unavailable during shutdown, preventing new work from
   starting while resources are being freed.

3. Set up httplib exception handler: Catches any internal httplib
   exceptions and returns HTTP 500 instead of crashing.

4. Change lambda captures from [&amp;] to [this]: More explicit about what's
   captured, making the code's intent clearer.

5. Delete copy constructor/assignment: Prevents unsafe copying since
   lambdas capture 'this' pointer.

The second crash was occurring in std::__tree::__assign_multi during
httplib::Server::process_request, caused by accessing freed memory when
the Impl object was destroyed while worker threads were still running.

* fix: ensure thread pool is joined before resources are destroyed

Root cause: C++ member destruction order was causing use-after-free.
The m_content map was being destroyed BEFORE m_server, but m_server's
destructor is what joins the thread pool threads. This meant worker
threads could still be accessing m_content after it was destroyed.

Crashes were occurring in httplib's internal map operations
(__construct_node, __assign_multi) during process_request because
threads were accessing freed memory.

Fix:
1. Changed m_server to unique_ptr&lt;httplib::Server&gt; to enable explicit
   destruction timing control.

2. In destructor, explicitly destroy server via m_server.reset() BEFORE
   other members are destroyed. This ensures thread pool threads are
   fully joined first.

3. In stop(), also destroy the server to ensure threads are joined
   before clearing content.

4. Reordered member declarations: m_server is now declared LAST so if
   we miss any explicit destruction, the natural destruction order will
   still destroy it first (reverse of declaration order).

This is a critical fix for the SIGSEGV crashes in httplib's internal
map operations during request processing on Android.

* format

---------

Co-authored-by: Claude &lt;noreply@anthropic.com&gt;
Co-authored-by: Andreas Stefl &lt;stefl.andreas@gmail.com&gt;
diff --git a/src/odr/http_server.cpp b/src/odr/http_server.cpp
@@ -8,46 +8,111 @@
 
 #include <httplib/httplib.h>
 
+#include <atomic>
+#include <memory>
+#include <sstream>
+
 namespace odr {
 
 class HttpServer::Impl {
 public:
   Impl(Config config, std::shared_ptr<Logger> logger)
-      : m_config{std::move(config)}, m_logger{std::move(logger)} {
-    m_server.Get("/",
-                 [](const httplib::Request & /*req*/, httplib::Response &res) {
-                   res.set_content("Hello World!", "text/plain");
-                 });
-
-    m_server.Get("/file/" + std::string(prefix_pattern),
-                 [&](const httplib::Request &req, httplib::Response &res) {
-                   serve_file(req, res);
-                 });
-    m_server.Get("/file/" + std::string(prefix_pattern) + "/(.*)",
-                 [&](const httplib::Request &req, httplib::Response &res) {
-                   serve_file(req, res);
-                 });
+      : m_config{std::move(config)}, m_logger{std::move(logger)},
+        m_server{std::make_unique<httplib::Server>()} {
+    // Set up exception handler to catch any internal httplib exceptions.
+    // This prevents crashes when exceptions occur during request processing.
+    m_server->set_exception_handler([this](const httplib::Request & /*req*/,
+                                           httplib::Response &res,
+                                           std::exception_ptr ep) {
+      try {
+        if (ep) {
+          std::rethrow_exception(ep);
+        }
+      } catch (const std::exception &e) {
+        ODR_ERROR(*m_logger, "Exception in HTTP handler: " << e.what());
+      } catch (...) {
+        ODR_ERROR(*m_logger, "Unknown exception in HTTP handler");
+      }
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    });
+
+    m_server->Get("/",
+                  [](const httplib::Request & /*req*/, httplib::Response &res) {
+                    res.set_content("Hello World!", "text/plain");
+                  });
+
+    m_server->Get("/file/" + std::string(prefix_pattern),
+                  [this](const httplib::Request &req, httplib::Response &res) {
+                    if (m_stopping.load(std::memory_order_acquire)) {
+                      res.status = 503;
+                      res.set_content("Service Unavailable", "text/plain");
+                      return;
+                    }
+                    serve_file(req, res);
+                  });
+    m_server->Get("/file/" + std::string(prefix_pattern) + "/(.*)",
+                  [this](const httplib::Request &req, httplib::Response &res) {
+                    if (m_stopping.load(std::memory_order_acquire)) {
+                      res.status = 503;
+                      res.set_content("Service Unavailable", "text/plain");
+                      return;
+                    }
+                    serve_file(req, res);
+                  });
+  }
+
+  ~Impl() {
+    // Ensure the server is properly stopped before destruction.
+    // This prevents crashes from worker threads accessing freed memory.
+    //
+    // IMPORTANT: We must fully stop and destroy the server BEFORE
+    // m_content is destroyed. httplib's thread pool threads may still
+    // be running after stop() returns - they only fully stop when the
+    // Server destructor joins them. By explicitly destroying the server
+    // here (via unique_ptr::reset), we ensure all threads are joined
+    // before any other members are destroyed.
+    m_stopping.store(true, std::memory_order_release);
+    if (m_server) {
+      m_server->stop();
+      m_server.reset(); // Destroy server, join all thread pool threads
+    }
+    // Now safe to let other members destruct - no threads are running
   }
 
+  // Prevent copying - the lambdas capture 'this' so copying would be unsafe
+  Impl(const Impl &) = delete;
+  Impl &operator=(const Impl &) = delete;
+
   const Config &config() const { return m_config; }
 
   const std::shared_ptr<Logger> &logger() const { return m_logger; }
 
   void serve_file(const httplib::Request &req, httplib::Response &res) {
-    std::string id = req.matches[1].str();
-    std::string path = req.matches.size() > 1 ? req.matches[2].str() : "";
-
-    std::unique_lock lock{m_mutex};
-    auto it = m_content.find(id);
-    if (it == m_content.end()) {
-      ODR_ERROR(*m_logger, "Content not found for ID: " << id);
-      res.status = 404;
-      return;
+    try {
+      std::string id = req.matches[1].str();
+      std::string path = req.matches.size() > 1 ? req.matches[2].str() : "";
+
+      std::unique_lock lock{m_mutex};
+      auto it = m_content.find(id);
+      if (it == m_content.end()) {
+        ODR_ERROR(*m_logger, "Content not found for ID: " << id);
+        res.status = 404;
+        return;
+      }
+      auto [_, service] = it->second;
+      lock.unlock();
+
+      serve_file(res, service, path);
+    } catch (const std::exception &e) {
+      ODR_ERROR(*m_logger, "Error handling request: " << e.what());
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    } catch (...) {
+      ODR_ERROR(*m_logger, "Unknown error handling request");
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
     }
-    auto [_, service] = it->second;
-    lock.unlock();
-
-    serve_file(res, service, path);
   }
 
   void serve_file(httplib::Response &res, const HtmlService &service,
@@ -60,17 +125,27 @@ class HttpServer::Impl {
 
     ODR_VERBOSE(*m_logger, "Serving file: " << path);
 
-    httplib::ContentProviderWithoutLength content_provider =
-        [service = service, path = path](const std::size_t offset,
-                                         httplib::DataSink &sink) -> bool {
-      if (offset != 0) {
-        throw std::runtime_error("Invalid offset: " + std::to_string(offset) +
-                                 ". Must be 0.");
-      }
-      service.write(path, sink.os);
-      return false;
-    };
-    res.set_content_provider(service.mimetype(path), content_provider);
+    // Buffer content to avoid streaming issues on Android.
+    // Using ContentProviderWithoutLength (chunked transfer encoding) can cause
+    // SIGSEGV crashes in httplib::Server::write_response_core when:
+    // 1. The client disconnects during transfer
+    // 2. Exceptions are thrown during content generation
+    // 3. The server is stopped while requests are in-flight
+    // By buffering content first, we can handle errors gracefully and use
+    // Content-Length based responses which are more reliable.
+    try {
+      std::ostringstream buffer;
+      service.write(path, buffer);
+      res.set_content(buffer.str(), service.mimetype(path));
+    } catch (const std::exception &e) {
+      ODR_ERROR(*m_logger, "Error serving file " << path << ": " << e.what());
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    } catch (...) {
+      ODR_ERROR(*m_logger, "Unknown error serving file: " << path);
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    }
   }
 
   void connect_service(HtmlService service, const std::string &prefix) {
@@ -88,7 +163,7 @@ class HttpServer::Impl {
   void listen(const std::string &host, const std::uint32_t port) {
     ODR_VERBOSE(*m_logger, "Listening on " << host << ":" << port);
 
-    m_server.listen(host, static_cast<int>(port));
+    m_server->listen(host, static_cast<int>(port));
   }
 
   void clear() {
@@ -107,17 +182,32 @@ class HttpServer::Impl {
   void stop() {
     ODR_VERBOSE(*m_logger, "Stopping HTTP server...");
 
-    clear();
+    // Set stopping flag first to reject new requests immediately.
+    // This prevents new requests from starting while we're shutting down.
+    m_stopping.store(true, std::memory_order_release);
+
+    if (m_server) {
+      // Stop the server to prevent new connections.
+      // Note: httplib::Server::stop() signals shutdown but thread pool
+      // threads may still be running. They only fully stop when the
+      // Server is destroyed. For explicit stop() calls (not destructor),
+      // we destroy the server here to ensure threads are joined.
+      m_server->stop();
+      m_server.reset(); // Destroy server, join all thread pool threads
+    }
 
-    m_server.stop();
+    // Clear content after server is fully destroyed to avoid use-after-free.
+    clear();
   }
 
 private:
   Config m_config;
 
   std::shared_ptr<Logger> m_logger;
 
-  httplib::Server m_server;
+  // Flag to indicate server is shutting down - checked by handlers
+  // to reject new requests during shutdown.
+  std::atomic<bool> m_stopping{false};
 
   struct Content {
     std::string id;
@@ -126,6 +216,14 @@ class HttpServer::Impl {
 
   std::mutex m_mutex;
   std::unordered_map<std::string, Content> m_content;
+
+  // IMPORTANT: m_server is declared LAST and as unique_ptr so we can
+  // explicitly destroy it in the destructor BEFORE other members.
+  // httplib's Server destructor joins thread pool threads, so we must
+  // ensure threads are fully stopped before m_content is destroyed.
+  // Using unique_ptr allows us to call reset() to trigger destruction
+  // at a controlled point in the destructor.
+  std::unique_ptr<httplib::Server> m_server;
 };
 
 HttpServer::HttpServer(const Config &config, std::shared_ptr<Logger> logger)
