1.5.0 regression: SIGSEGV on MSVC/Windows the moment a WebSocket client connects (clean on 1.4.0)

[binaryduke]

Summary

1.5.0 segfaults on MSVC / Windows (IOCP) the moment a WebSocket client connects to a ws::server.

1.4.0 is clean with identical source. Reproduced with your own examples/15-WS.cpp + examples/16-WS-Client.cpp, unmodified, against a pristine git worktree of tag 1.5.0 (60290b4) — git status clean, no local patches of any kind.

Flagging it because 1.5.0 is tagged stable and frozen, and — from #35 — you're testing on MinGW + Wine, so this may be invisible on your side. We have a real MSVC box and are happy to run anything you want against it.

Environment

MSVC 2022 (v19.x), x64, /std:c++20 /EHsc /GR /O2 /MT, -DNOMINMAX -DWIN32_LEAN_AND_MEAN. OpenSSL + zlib from vcpkg x64-windows-static.

Result

Pristine tag 1.5.0 (60290b4):

[t= 0 ms] server started at http://localhost:8000
[t= 511 ms] alive
[t= 1012 ms] alive
[t= 1513 ms] alive
[t= 2014 ms] alive
[t= 2280 ms] connected
SIGSEGV: Segmentation Violation
[t= 2280 ms] closed

Control — identical source, identical compiler and flags, only the nodepp include path changed — pristine tag 1.4.0 (e9b1866):

[t= 2073 ms] connected
ping from client                      <-- client -> server data arrives fine
[t= 8073 ms] closed
[t= 10002 ms] survived to 10s -- clean exit

tree	WS client connects
1.4.0 (e9b1866)	clean — bidirectional traffic, 10s, exit 0
1.5.0 (60290b4)	SIGSEGV, immediately, every run

Layer isolation on 1.5.0: an HTTP client hitting a plain http::server is clean. The crash only appears once ws::server() is layered on. So the accept/socket path is healthy; it's the WS upgrade path.

Where we'd look — a suspect, not a diagnosis

1.5.0 changed onSocket from event_t<socket_t> to event_t<ptr_t<tcp_t>, socket_t> (tcp.h:44, tls.h:45), and ws::server() now does its work inside a deferred process::add(...) task that captures that self pointer:

skt.onSocket([=]( ptr_t<tcp_t> self, socket_t raw ){
    http_t hrv = raw;
    if( !generator::ws::server( hrv ) ){ self->onConnect.skip(); return; }
    ws_t cli = raw;
    process::add([=](){
        cli.set_timeout(0); cli.resume();
        cli.set_mask(0);
        self->onConnect.resume();
        self->onConnect.emit(cli);   // <-- self used after onSocket has already returned
        stream::pipe(cli);
    return -1; });
});

The crash lands right at onConnect. self being dereferenced from the deferred task, after the onSocket handler has returned, is our first suspect — but we have no debugger on this box and have not confirmed it. Treat it as a pointer, not a conclusion.

One trap worth knowing about

nodepp's own SIGSEGV handler (signal.h:36) prints and then calls process::exit(0) — so a crashed process still exits with status 0. Any CI or test harness grading on exit code will score a segfault as a pass. It briefly fooled us. Grade on the log line.

Repro

Your examples/15-WS.cpp and examples/16-WS-Client.cpp, split into two processes so the client is genuinely separate, with a heartbeat and a timed exit added so the run terminates. Nothing else changed.

server (your 15-WS.cpp)

#include <nodepp/nodepp.h>
#include <nodepp/timer.h>
#include <nodepp/http.h>
#include <nodepp/ws.h>

using namespace nodepp;

static ulong T0 = 0;
static void mark( const char* w ){ console::log("[t=",(ulong)(process::millis()-T0),"ms]",string_t(w)); }

void onMain() {
    T0 = process::millis();

    auto server = http::server([=]( http_t cli ){
        cli.write_header( 200, header_t({ { "Content-Security-Policy", "*" } }) );
        cli.write("Hello World!");
    }); ws::server( server );

    server.onConnect([=]( ws_t cli ){
        mark("connected");
        cli.onData([=]( string_t data ){ cli.write( "<: received" ); console::log( data ); });
        cli.onClose([=](){ mark("closed"); });
    });

    server.onError([=]( except_t err ){ console::log( ">>", err.what() ); });

    server.listen( "localhost", 8000, [=]( socket_t server ){
        mark("server started at http://localhost:8000");
    });

    timer::interval( [](){ mark("alive"); }, 500 );
    timer::timeout ( [](){ mark("survived to 10s -- clean exit"); process::exit(0); }, 10000 );
}

client (your 16-WS-Client.cpp)

#include <nodepp/nodepp.h>
#include <nodepp/timer.h>
#include <nodepp/ws.h>

using namespace nodepp;

void onMain() {
    auto client = ws::client( "ws://localhost:8000/" );

    client.onConnect([=]( ws_t cli ){
        console::log("connected" );
        cli.onData([]( string_t data ){ console::log( data ); });
        cli.onClose([=](){ console::log("closed"); });
        timer::timeout([=](){ ws_t c = cli; c.write("ping from client"); }, 500 );
    });

    client.onError([=]( except_t err ){ console::log( "<>", err.data() ); });
    timer::timeout( [](){ console::log("client done"); process::exit(0); }, 6000 );
}

Start the server, then run the client.

[binaryduke]

Correction: our "dangling self" suspect was wrong. Please ignore it. type::bind() deep-copies, so self never dangles. We flagged it as a suspect rather than a diagnosis, and it was the wrong tree.

We have the real root cause now, with evidence — and a working fix. It has nothing to do with onSocket's new signature. It's in windows/kernel.h, and it's a nice one.

Root cause: IOCP double-association

1.5.0 added poll-descriptor caching — get_pd() / pd on the shared socket node. The bug is that remove() erases the kv_queue bookkeeping node but never resets pd, so pd is left pointing at freed memory.

ws::server() registers the same socket twice: once for the handshake read, and again for stream::pipe(cli) in the deferred task. On that second registration:

1. poll_add() reads the stale pd. queue_t::as() correctly rejects it as invalid.
2. So poll_add() takes the "register a new entry" branch and calls append().
3. append() calls CreateIoCompletionPort() on a handle that is already associated.
4. Windows always fails that — ERROR_INVALID_PARAMETER. A handle can only ever be associated with an IOCP once, even to the same port.
5. append() treats it as fatal, erases the node, and returns the 0x01 sentinel.
6. poll_add() dereferences that sentinel unconditionally: kv_queue.as(task->addr)->data.event.add(...) where task->addr == (void*)0x01.

A deterministic near-null write. Verbatim, from instrumenting a throwaway copy of pristine 1.5.0:

DBG-P: poll_add entry, ... inp.get_pd()= 0000021BD99CE0A0
DBG-P: existing kv found for pd?  0
DBG-K: append() fd= ...0158  new_node= ...CE410  kv_queue.size= 2
DBG-K: append() CreateIoCompletionPort FAILED, erasing node= ...CE410
DBG-P: after append(), inp.get_pd()= 0000000000000001
DBG-P: about to deref kv_queue.as(task->addr= 0000000000000001 )
SIGSEGV: Segmentation Violation

That's why plain HTTP is clean and only the WS upgrade crashes — HTTP never registers the same socket a second time. And it's why 1.4.0 is unaffected: it predates get_pd()/pd entirely.

The fix we're running

include/nodepp/windows/kernel.h, two parts:

@@ append()
-        if( !CreateIoCompletionPort( id->data.fd, obj->pd, (ULONG_PTR)id, 0 ) )
-          { obj->kv_queue.erase(id); return 0x01; }
+        if( !CreateIoCompletionPort( id->data.fd, obj->pd, (ULONG_PTR)id, 0 ) ) {
+            // A handle can only ever be IOCP-associated ONCE. A second
+            // poll_add() on the same still-open socket lands here with
+            // ERROR_INVALID_PARAMETER. The OS-level association from the first
+            // registration is still valid -- only our bookkeeping node was lost.
+            // Keep the new node instead of discarding it.
+            if( GetLastError() == ERROR_INVALID_PARAMETER ){ return (uchar_64) id; }
+            obj->kv_queue.erase(id); return 0x01;
+        }

@@ poll_add()
-        obj->kv_queue.as(task->addr)->data.event.add([=](){
-            return clb( args... )>=0 ? 1 : -1;
+        // append() can still legitimately return the 0x01 sentinel. Don't
+        // dereference a lookup that is allowed to be nullptr.
+        auto found = obj->kv_queue.as(task->addr);
+        if( found == nullptr ){ return nullptr; }
+
+        found->data.event.add([=](){
+            return clb( args... )>=0 ? 1 : -1;
         }); invoker( task->addr );

Verified on MSVC 2022, x64, /std:c++20 /O2 /MT, against your own examples/15-WS.cpp + 16-WS-Client.cpp, unmodified:

• 3/3 clean single-client runs — connect, bidirectional traffic, survive the example's 10s timer, exit 0, no SIGSEGV line.
• 5 concurrent WS clients on one server: all connect, all exchange data both ways, all close cleanly.
• 1.4.0 unaffected.
• And our own product's WebSocket server (which is what found this) now accepts and broadcasts to real clients again.

Two honest caveats

1. Ours is a fix, not necessarily the fix. Keeping the node means that second registration is driven by the level-triggered invoker(nullptr) round-robin each tick rather than by a prompt IOCP completion wakeup. It's correct, but it's a slightly slower path for re-registered sockets. You may well prefer to attack it further up — e.g. have remove() reset pd to 0, and/or track the association so append() never attempts the second CreateIoCompletionPort() at all. You know this scheduler far better than we do. Treat our patch as a starting point and a proof of the mechanism.

2. The null guard in poll_add() is worth keeping regardless of how you fix the root cause — that unconditional dereference of a documented failure sentinel is a landmine independent of what puts the sentinel there.

Offer

Happy to open this as a PR — same question as #33: main, or the new dev?

And the standing offer: we have a real MSVC 2022 / Windows box and you don't. Send us any candidate and we'll run it. Given how quickly you turned #32 and #35 around, the least we can do is be your Windows CI.

[EDBCREPO]

IOCP — fixed on dev

You almost gave me a heart attack. ptr_t is supposed to be the safest component in Nodepp (or at least it should be).

Why 1.4.0 works and 1.5.0 didn't

I forgot to add the fallback logic to the kernel (for some reason). Here's the mechanism:

It's impossible for a stream to be registered twice — that's what if( obj->kv_queue.as( (void*) inp.get_pd() )==nullptr ) prevents. But there are cases where IOCP fails to register the HANDLER, and the kernel marks it as KV_STATE_FALLBACK or 0x01:

uchar_64 append( kevent_t kv ) const noexcept {
    obj->kv_queue.push( kv ); 
    auto id = obj->kv_queue.last();

    if( !CreateIoCompletionPort( id->data.fd, obj->pd, (ULONG_PTR)id, 0 ) ) {
        obj->kv_queue.erase(id); return FLAG::KV_STATE_FALLBACK; 
    }

    return (uchar_64) id;
}

When it fails and returns KV_STATE_FALLBACK, poll_add must returns an asynchronous task, preventing the stream from getting stuck:

template< class T, class U, class... W >
ptr_t<task_t> poll_add( T& inp, int flag, U cb, ulong timeout=0, const W&... args ) const noexcept {
    function_t<int,W...> clb ( cb ); 
    if( inp.is_closed() ){ return nullptr; }

    if( inp.get_pd() == FLAG::KV_STATE_FALLBACK ){ 
        if( is_std( (HANDLE) inp.get_fd() ) ) {
            return loop_add( coroutine::add( COROUTINE(){
                coBegin; 
                while( clb( args... ) >= 0 ) {
                    coDelay( 100 );
                }
                coFinish
            }));
        } else { 
            return loop_add( cb, args... ); 
        }
    }

    if( obj->kv_queue.as( (void*) inp.get_pd() ) == nullptr ) {
        kevent_t kv;
        kv.flag    = flag; 
        kv.fd      = (HANDLE) inp.get_fd();
        ulong time = timeout == 0 ? 0 : process::now() + timeout;
        
        kv.callback = [=](){
            if( time != 0 && time < process::now() ) {
                inp.close(); 
                return -1; 
            }
            if( inp.is_closed() ) { return -1; } 
            if( inp.is_waiting() ) { return 0; }
            return 1; 
        };

        inp.get_pd() = append( kv );

        if( inp.get_pd() == KV_STATE_FALLBACK ) {
            return poll_add( inp, flag, cb, timeout, args... );
        }
    }

    ptr_t<task_t> task( 0UL, task_t() );
    task->addr  = (void*) inp.get_pd();
    task->flag  = TASK_STATE::OPEN; 
    task->sign  = &obj;
    
    obj->kv_queue.as( task->addr )->data.event.add([=](){ 
        return clb( args... ) >= 0 ? 1 : -1; 
    }); 
    invoker( task->addr );

    return task;
}

The fix

These changes are now available in the dev branch:

https://github.com/NodeppOfficial/nodepp/tree/dev

Offer

I appreciate the MSVC testing — it's invaluable. If you can verify the dev branch on your Windows box, I'll merge it to main as soon as it's confirmed stable.

Thanks again for the detailed report and the root cause analysis. This one was a good catch.

[binaryduke]

Confirmed on MSVC 2022 x64 /std:c++20 /MT (with the one-line generator min() cast
in the linked PR, without which the WS TU does not build on MSVC).

The KV_STATE_FALLBACK path resolves the connect-time SIGSEGV. A server+client
WebSocket test over workers connected both directions and exchanged data
bidirectionally for ~7s with zero SIGSEGV. The prior tagged build crashed on
connect on the same box; dev does not.

Good to merge to main from this side.