Skip to content

LibSocketAsio: fix amuleIPV4Address operator= leaking endpoint on every assign - #716

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/amule-ipv4address-operator-leak
May 25, 2026
Merged

LibSocketAsio: fix amuleIPV4Address operator= leaking endpoint on every assign#716
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/amule-ipv4address-operator-leak

Conversation

@got3nks

@got3nks got3nks commented May 25, 2026

Copy link
Copy Markdown
Contributor

Summary

amuleIPV4Address::operator= allocated a fresh CamuleIPV4Endpoint on every call without freeing the previously-held one. Every UDP packet the daemon receives leaks one endpoint (~28 bytes) because CMuleUDPSocket::OnReceive constructs a local amuleIPV4Address and then has RecvFrom assign into it at LibSocketAsio.cpp:1173 (addr = recdata->ipadr).

On a busy node this fires several times per second across Kad, source-exchange and server traffic. ASAN report on the 17 h trace in #712 shows 238 907 leaked endpoints (6.7 MB direct + matching indirect) from this single site, dominating every other leak in the report by two orders of magnitude — and scaling with shared content because larger sharesets receive more Kad OP_KADEMLIA2_PUBLISH_KEY_REQ/*_RES and source-lookup traffic.

Fix

  1. Move m_endpoint allocation into each constructor's member-init list so the class invariant m_endpoint != nullptr holds from construction onwards.
  2. Have operator= mutate the existing endpoint in place (*m_endpoint = …) instead of allocating a replacement. Self-assignment is guarded for the same-type overload.

Side benefit: removes a new/delete pair from the UDP receive hot path.

Test plan

  • macOS Debug build (-DBUILD_DAEMON=YES -DBUILD_AMULECMD=YES), clean compile
  • Smoke test: launched amuled, Kad bootstrap from 160 saved contacts, server connection attempts (each goes through the constructor + operator= path), graceful shutdown via amulecmd … Shutdown, no crash
  • Long-soak ASAN re-run by reporter on a busy daemon to confirm the dominant leak family is gone — tracked in Memory leak master branch #712

Refs #712.

…ry assign

The operator= overloads allocated a fresh CamuleIPV4Endpoint on every call
without freeing the previously-held one. Because CMuleUDPSocket::OnReceive
constructs a local amuleIPV4Address on the stack and then has RecvFrom assign
into it (LibSocketAsio.cpp:1173, addr = recdata->ipadr), each UDP datagram
leaks one CamuleIPV4Endpoint (~28 bytes). On a busy daemon this fires several
times per second from Kad, source-exchange and server traffic — ~9 MB/day on
the 17 h ASAN trace attached to amule-project#712.

Fix is to (a) initialise m_endpoint in every constructor's member-init list
so the class invariant 'm_endpoint != nullptr' holds from construction
onwards, and (b) have operator= mutate the existing endpoint in place rather
than allocating a replacement. Self-assignment is guarded for the same-type
overload.

This also removes a new/delete pair from the UDP receive hot path.

Refs amule-project#712.
@mrjimenez
mrjimenez merged commit 732bfcb into amule-project:master May 25, 2026
7 checks passed
@got3nks
got3nks deleted the fix/amule-ipv4address-operator-leak branch May 27, 2026 15:15
mrjimenez pushed a commit to mrjimenez/amule that referenced this pull request Jul 30, 2026
…ted copies (amule-project#716)

The EC headers existed twice: generated from their .abstract sources into
the build tree, and committed under src/libs/ec/cpp/ where they were
hand-maintained. Adding a tag meant editing the abstract AND hand-adding
the matching enum entry and name-string case to the committed header, with
nothing enforcing that the two agreed.

They did not agree in one respect already: `#include <cstdint>` was added
to the committed header by 5a6f5e8, and the generator never learned
about it.

Worse, both copies reached the same binary. Measured with ninja's
dependency data on master, 24 TUs compiled against the committed ECCodes.h
and 3 (Api.cpp, PrefsSchema.cpp, Refresher.cpp) against the generated one,
with amuleapi linking objects of both kinds. Divergence there would not
have been a compile error -- it would have linked cleanly and put two
different values for the same tag on the EC wire.

Make the generated header the only one. The abstract becomes the single
source of truth: edit it and the enum entry, the DEBUG_EC_IMPLEMENTATION
name string and the documentation all follow.

The generator had to learn three things first, so the output is not a
regression on the header it replaces:

  * emit `#include <cstdint>` when the abstract has a TypeDef section
    (those emit `typedef uint8_t ec_opcode_t`). ECTagTypes has none and
    uses aMule's own uint8, so it does not get the include.
  * `##` is a documentation comment, emitted into the header above the
    entry that follows it. Plain `#` keeps its meaning of an
    abstract-only note. The 28 comment lines the committed header carried
    move into the abstract, and come out byte-identical.
  * a literal `;` no longer corrupts the output. The file is turned into
    a CMake list by swapping newlines for `;`, so a semicolon in prose
    split one line into several and the fragments were parsed as entries.
    The same unquoted-variable bug was silently eating semicolons in the
    licence block ("free software; you can" -> "free software you can").

Verified against the header being deleted: enum name=value pairs and
typedefs are identical, and every documentation comment is preserved
verbatim. The only remaining differences are cosmetic -- brace placement
and the switch-case layout in the debug block, neither of which any
consumer sees now that the file lives solely in the build tree.

All 86 TUs that include ECCodes.h and all 84 that include ECTagTypes.h now
resolve to the generated copy; none to a source-tree one. A cold parallel
build from an empty tree succeeds, so the ordering holds without a race.

The ec/java/ counterparts are deleted too: nothing built, shipped or
referenced them, and they were only ever updated by hand alongside the
headers.
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.

2 participants