Skip to content

Fix reconnect leaks and harden memory handling - #406

Merged
si458 merged 1 commit into
Ylianst:masterfrom
Rambomst:fix-reconnect-leaks-and-memory-handling
Sep 17, 2026
Merged

si458 merged 1 commit into
Ylianst:masterfrom
Rambomst:fix-reconnect-leaks-and-memory-handling

Conversation

@Rambomst

@Rambomst Rambomst commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Cancelling a WebSocket connection could leave the connection object, queued request, SNI data, and fragment buffer allocated. The network timeout handler could also start a second connection attempt after cancellation had already scheduled the next retry.

This PR:

  • Releases orphaned WebSocket state when a connection is cancelled.
  • Leaves reconnect scheduling to the existing cancellation response path.
  • Prevents Duktape from inspecting allocator metadata in undersized buffers.
  • Bounds Unix Buffer.toString() conversions to the actual buffer length.
  • Reserves the alignment padding required by stack allocations.
  • Uses aligned allocation sizes when tracking Duktape memory.
  • Corrects thread cleanup and timeout handling on systems without pthread_timedjoin_np().

Relates to #110

Please follow this checklist to avoid unnecessary back and forth (click to expand)
  • 🧠 I used LLMs/AI in this contribution and reviewed all generated content.
    I understand that I am responsible for and able to explain every line of code I submit.
  • 🛠️ I have self-reviewed my code and self-tested it against a MeshCentral server to ensure it works as expected.
  • 🖥️ My change compiles on every platform it affects (Windows / Linux / macOS / FreeBSD), and I have considered
    the impact on platforms and architectures I could not test.
  • 📦 If I changed JavaScript modules under modules/, I re-embedded them so the compiled-in copies in
    microscript/ILibDuktape_Polyfills.c match (the agent runs the embedded copies, not the files on disk).
  • 🤖 I ran the agent self-test where appropriate (see "Self Test" in readme.md).
  • 📄 Documentation updates are included (if applicable), e.g. the .msh options table in readme.md.
  • 🧰 Updates to vendored dependencies (OpenSSL, zlib, ...) are listed and explained.
  • ⚠️ CI passes and is green (Windows / Linux / macOS / FreeBSD builds and CodeQL).

Testing

I tested the changes in disposable VMs against an isolated MeshCentral 1.2.5 server.

Platform (OS / distro / arch) Tested Result
Debian 12 x86-64, glibc 2.36 Debug builds passed reconnect, cancellation, buffer, stack, and thread lifecycle tests under Valgrind and AddressSanitizer.
Ubuntu 24.04.5 x86-64, glibc 2.39 The full release build passed stalled TLS, WebSocket reconnect, cancellation, buffer, thread lifecycle, and server restart tests.
Alpine 3.24.1 x86-64, musl 1.2.6 The ARCHID=33 build passed reconnect, cancellation, buffer, and thread lifecycle tests under Valgrind.
Windows 11 x64 Debug and Release builds passed reconnect, buffer boundary, stack runtime, and CRT heap checks.
FreeBSD 14.4 amd64 The full KVM release build compiled. Focused cancellation and thread lifecycle tests completed without lost blocks.
macOS I did not have a macOS guest available. The shared fallback thread implementation was exercised on Alpine and FreeBSD.

The main before and after results were:

  • In the historical proxy test, disabling the existing GC call caused memory to grow from about 3.6 MB to 890 MB over 2,000 failed connections. With the GC call present, memory settled around 4.8 MB.
  • The current WebSocket cancellation path retained about 119 KB over 100 cancellations on both Debian and Windows. After the fix, Valgrind and the Windows CRT heap check reported no retained allocations.
  • The cancellation regression completed 500 callbacks, including 100 established WebSockets with incomplete fragments.
  • AddressSanitizer reproduced a stack buffer overflow in the original allocator. The corrected allocator passed every payload size from 0 through 257.
  • On musl, the original thread fallback retained 88 bytes for every detached thread. The corrected implementation completed 100 detached workers, timed joins, and blocking joins without a leak.
  • Normal stalled TLS tests ran for just over a minute on Ubuntu, Alpine, and Windows. Each completed two timeout and retry cycles and exited cleanly.
  • Debian completed 100 accelerated timeout cycles without Valgrind errors or retained allocations.

Release orphaned WebSocket connection state during cancellation and avoid
starting a second reconnect from the timeout handler.

Correct Duktape buffer bounds and allocation accounting, reserve aligned
stack storage, and fix thread wrapper ownership on platforms without
pthread_timedjoin_np().
@PTR-inc

PTR-inc commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Did a basic test on macos arm64: connection to server, files/desktop/terminal/details tabs work.

@PTR-inc

PTR-inc commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

..and did a valgrind/stresstest on mac arm64, all good.

@si458
si458 merged commit 29ce82c into Ylianst:master Sep 17, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants