uNetworking / uNetworking/uWebSockets

Double-Free of UserData when gracefully closing a Websocket from Server-Side

Open
#1,863 8 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

medium priority
Dominant language
C++
Stars
19k
Forks
1.9k
Avg merge
3h 24m
Merged PRs (30d)
5

Description

Hi,
i am trying to implement a graceful server Shutdown. Either i am doing something wrong, or there is a fundamental bug in the implementation. ASAN complains (rightfully) about a double-free attempt.

My Shutdown-Code involves:

loop->defer([this]
{
    for (auto* con: websocketConnections)
        con->end(1000, "Server shutting down");
    if (appPtr)
        appPtr->close();
});

The code of con->end() currently finishes with the following lines:

        /* Emit close event */
        if (webSocketContextData->closeHandler) {
            webSocketContextData->closeHandler(this, code, message);
        }
        ((USERDATA *) this->getUserData())->~USERDATA();

As we can see, the destructor of UserData is called at the very end.

However webSocketContextData->closeHandler() already brings me into uWS::TemplatedApp::ws in App.hpp, line 263 - 439.
Within this function there is this snippet:

        /* Copy all handlers */
        webSocketContext->getExt()->openHandler = std::move(behavior.open);
        webSocketContext->getExt()->messageHandler = std::move(behavior.message);
        webSocketContext->getExt()->drainHandler = std::move(behavior.drain);
        webSocketContext->getExt()->subscriptionHandler = std::move(behavior.subscription);
        webSocketContext->getExt()->closeHandler = std::move([closeHandler = std::move(behavior.close)](WebSocket<SSL, true, UserData> *ws, int code, std::string_view message) mutable {
            if (closeHandler) {
                closeHandler(ws, code, message);
            }

            /* Destruct user data after returning from close handler */
            ((UserData *) ws->getUserData())->~UserData();   // <<<<<<<<<<<<<<<<<<<<<<<<<-----
        });
        webSocketContext->getExt()->pingHandler = std::move(behavior.ping);
        webSocketContext->getExt()->pongHandler = std::move(behavior.pong);

Where i have marked the destructor-call.

So, i am running into a code-path where the destructor is definitely called twice.

This is the full call-stack

[nelson] <lambda#1>::operator()(uWS::WebSocket<true, true, WebsocketData> *, int, std::string_view) App.h:383 <----------------- is calling the destructor
[nelson] std::__invoke_impl<void, <lambda#1> &, uWS::WebSocket<true, true, WebsocketData> *, int, std::string_view>(std::__invoke_other, <lambda#1> &, uWS::WebSocket<true, true, WebsocketData> *&&, int &&, std::string_view &&) invoke.h:63
[nelson] std::__invoke_r<void, <lambda#1> &, uWS::WebSocket<true, true, WebsocketData> *, int, std::string_view>(<lambda#1> &, uWS::WebSocket<true, true, WebsocketData> *&&, int &&, std::string_view &&) invoke.h:113
[nelson] std::move_only_function<void (uWS::WebSocket<true, true, WebsocketData> *, int, std::string_view)>::_S_invoke<<lambda#1> >(std::_Mofunc_base *, uWS::WebSocket<true, true, WebsocketData> *, int, std::string_view &&) mofunc_impl.h:219
[nelson] std::move_only_function<void (uWS::WebSocket<true, true, WebsocketData> *, int, std::string_view)>::operator() mofunc_impl.h:184
[nelson] uWS::WebSocket<true, true, WebsocketData>::end WebSocket.h:266  <----------------- will call destructor again once this callstack finishes.
[nelson] <lambda#1>::operator()() const server.cpp:234
[nelson] std::__invoke_impl<void, <lambda#1> &>(std::__invoke_other, <lambda#1> &) invoke.h:63
[nelson] std::__invoke_r<void, <lambda#1> &>(<lambda#1> &) invoke.h:113
[nelson] std::move_only_function<void ()>::_S_invoke<<lambda#1> >(std::_Mofunc_base *) mofunc_impl.h:219
[nelson] std::move_only_function<void ()>::operator() mofunc_impl.h:184
[nelson] uWS::Loop::wakeupCb Loop.h:50
[nelson] us_loop_run epoll_kqueue.c:147
[nelson] uWS::Loop::run Loop.h:216
[nelson] uWS::run Loop.h:233
[nelson] uWS::TemplatedApp<true>::run App.h:571
[nelson] ServerImpl::runApp server.cpp:215
[nelson] std::__invoke_impl<void, void (ServerImpl::*)(std::string), ServerImpl *, std::string> invoke.h:76
[nelson] std::__invoke<void (ServerImpl::*)(std::string), ServerImpl *, std::string> invoke.h:98
[nelson] std::thread::_Invoker<std::tuple<void (ServerImpl::*)(std::string), ServerImpl *, std::string> >::_M_invoke<0ul, 1ul, 2ul> std_thread.h:303
[nelson] std::thread::_Invoker<std::tuple<void (ServerImpl::*)(std::string), ServerImpl *, std::string> >::operator() std_thread.h:310
[nelson] std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (ServerImpl::*)(std::string), ServerImpl *, std::string> > >::_M_run std_thread.h:255
[libstdc++.so.6] std::execute_native_thread_routine 0x00007ffff64e51a4
[libasan.so.8] asan_thread_start 0x00007ffff785e29b
[libc.so.6] <unknown> 0x00007ffff62a57eb
[libc.so.6] <unknown> 0x00007ffff632918c

I setup the Websocket-functionality with

        // WebSocket handlers
        app.ws<WebsocketData>("/:0", {
            .upgrade = std::bind_front(&ServerImpl::upgradeWebsocket, this),
            .open = std::bind_front(&ServerImpl::onWebsocketOpen, this),
            .message = [](auto *ws, std::string_view msg, uWS::OpCode op)
            {
                ws->send(msg, op); // Echo
            },
            .close = std::bind_front(&ServerImpl::onWebsocketClose, this)
        });

With WebsocketData containing a std::string which is attempted to be destructed twice.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in WebSocket.h around WebSocket::end and follow the close-handler path into App.h around lines 263-439, especially the user-data destructor call. Reproduce the graceful shutdown with ASAN using the WebsocketData example, then verify that the close path destroys UserData only once and no longer reports the double-free.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.