HttpServer has no safe teardown: stop() double-closes the socket, and destroying it races listen()

[andiwand]

Destroying an HttpServer while another thread is inside listen() is a use-after-free, and the API gives the caller no way to avoid it.

Found while chasing an intermittent Process crashed in OpenDocument.droid's instrumented tests (odrcore 6.0.1, android x86_64 API 32, emulator). It is not android specific — the same race is reachable from plain C++ and from the JNI bindings.

The crash

Fatal signal 11 (SIGSEGV), code 128 (SI_KERNEL), fault addr 0x0 in tid 7413 (Thread-4)
  #00 libodr_jni.so (httplib::Server::listen_internal()+1069)
  #01 libodr_jni.so (odr::HttpServer::Impl::listen() const+532)
  #02 libodr_jni.so (Java_app_opendocument_core_HttpServer_listenNative+16)

The app calls stop() and then close() on the java HttpServer; the thread that had called listen() segfaults a few seconds later, still inside httplib's accept loop.

Why

listen() blocks, so it can only be called on a thread of its own, and stop() does not wait for it:

// httplib.h
inline void Server::stop() {
  if (is_running_) {
    std::atomic<socket_t> sock(svr_sock_.exchange(INVALID_SOCKET));
    detail::shutdown_socket(sock);
    detail::close_socket(sock);
  }
}

It shuts the listening socket down; listen_internal() notices on its next turn and returns some time later. Meanwhile ~Impl frees what that thread is standing on:

// src/odr/http_server.cpp
~Impl() {
    m_stopping.store(true, std::memory_order_release);
    if (m_server != nullptr) {
      m_server->stop();
      m_server.reset(); // Destroy server, join all thread pool threads
    }
}

The comment is half right. httplib::Server::~Server() = default, so member destruction does join the thread pool — but the thread executing listen() is the caller's thread, which httplib neither owns nor can join. m_server.reset() therefore deletes the httplib::Server out from under listen_internal().

HttpServer::listen() not taking a reference to what it runs on is what makes it reachable:

void HttpServer::listen() const { m_impl->listen(); }   // no shared_ptr copy

m_impl is a shared_ptr<Impl>, but listen() borrows it. Destroying the HttpServer on another thread drops the last reference and runs ~Impl immediately, mid-listen().

The caller cannot currently avoid it

odr::HttpServer exposes connect_service, bind, listen, clear, stop — there is no is_running(), no wait_until_stopped(), no join. cpp-httplib has is_running() and wait_until_ready(); neither is wrapped. So a caller has no way to ask whether listen() has actually returned, and the only sound approach left is to join the thread it started itself and hope the timeout was generous enough.

The JNI bindings make it sharper

NativeResource implements AutoCloseable and frees the handle both on close() and from a phantom-reference queue. Two consequences:

• close() from any thread while a listen thread is running is this use-after-free. The idiomatic try (HttpServer s = new HttpServer()) { … } is unsafe for exactly the one method that has to run on another thread.
• "Just don't call close()" is not a workaround either: letting the wrapper become unreachable runs Destroyer.destroy() → destroy(handle) → the same thing, at a moment the caller does not control. (In practice the JNI frame of the in-flight listenNative call keeps the instance reachable, so the GC path is narrower than the explicit one — but it is not something a caller should have to reason about.)

There is no HttpServer.isRunning() on the java side either.

What we did in the app

Not a fix, just damage control — OpenDocument.droid#548. CoreLoader.close() now hands the teardown to a short-lived daemon thread that joins the listen thread with a timeout, calls HttpServer.close() only if that thread actually came back, and otherwise keeps the wrapper in a static list forever so that neither close() nor the reference queue can free it. Leaking one server beats a SIGSEGV, but it is guesswork built on a timeout.

Suggestions

1. Make listen() keep its Impl alive — auto impl = m_impl; impl->listen();. Then ~Impl cannot run, and m_server cannot be freed, until listen() returns. That alone closes the use-after-free for both the C++ and the JNI path, and is a one-line change.
2. Expose the state: is_running() (httplib already has it) and ideally a wait_until_stopped(timeout), so stop() can be followed by something better than a sleep.
3. Consider making stop() synchronous, or documenting loudly that it is not, and that ~HttpServer must not run while listen() is in flight.
4. Fix the ~Impl comment — destroying httplib::Server does not join the thread that called listen().

Happy to send a PR for (1) and (2) if that is the direction you want.

[andiwand]

There is a second, independent teardown bug here, which I only found because the workaround for the first one did not stop the crashes. This one is arguably worse: it corrupts unrelated file descriptors rather than just crashing.

stop() and listen_internal() both close the listening socket

Same test suite, same emulator, different abort:

signal 6 (SIGABRT), code -6 (SI_TKILL)
Abort message: 'fdsan: attempted to close file descriptor 101,
                expected to be unowned, actually owned by ZipArchive 0x73adcf9fe0'
  #02 pc 0000000000054028  libc.so (close+16)
  #03 pc 0000000000460fbc  libodr_jni.so (httplib::Server::listen_internal()+920)
  #04 pc 000000000045d89c  libodr_jni.so (odr::HttpServer::Impl::listen() const+460)
  #05 pc 00000000002f00cc  libodr_jni.so (Java_app_opendocument_core_HttpServer_listenNative+20)

Server::stop() closes the fd itself:

inline void Server::stop() {
  if (is_running_) {
    std::atomic<socket_t> sock(svr_sock_.exchange(INVALID_SOCKET));
    detail::shutdown_socket(sock);
    detail::close_socket(sock);      // <-- closed here
  }
}

and listen_internal() has its own close_socket paths on the way out of the accept loop. Whichever ordering the two threads take, the same descriptor number can be closed twice. Between the two closes the number is free for reuse, and in a JVM process something always wants a descriptor — here ART opened the apk and got 101, so the second close hit a ZipArchive. android's fdsan noticed and aborted the process. On a platform without fdsan this closes an unrelated file silently, which is a much nastier failure than a segfault.

Why this matters for the fix

The suggestion in the issue above — having listen() hold a shared_ptr copy of its Impl — fixes the use-after-free but not this. The double close happens entirely between stop() and listen_internal(), with the HttpServer alive the whole time. stop() needs to hand the socket over rather than close it (leave the close to whoever exits the accept loop), or the two need to agree on ownership some other way.

So odr::HttpServer currently has no safe teardown at all: stop() races the accept loop over the fd, and destruction races it over the object.

What we ended up doing in the app

We gave up on tearing the server down. It is created once per process on first use and never stopped; between documents we call clear() to drop the connected services and leave the socket listening, and the process exit reclaims it. That was already effectively true — nothing here ever outlived its process — and it sidesteps both races because both are teardown-only.

It also happens to be the simpler design: no join, no timeout, no reaper thread, no main-thread block in the android service teardown, and the port is bound once instead of once per service lifetime.

I mention it because it may be worth documenting as the recommended usage until the two races are fixed: bind and listen once, clear() between workloads, never stop().