LibSocketAsio: fix amuleIPV4Address operator= leaking endpoint on every assign - #716
Merged
mrjimenez merged 1 commit intoMay 25, 2026
Conversation
…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.
3 tasks
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
amuleIPV4Address::operator=allocated a freshCamuleIPV4Endpointon every call without freeing the previously-held one. Every UDP packet the daemon receives leaks one endpoint (~28 bytes) becauseCMuleUDPSocket::OnReceiveconstructs a localamuleIPV4Addressand then hasRecvFromassign into it atLibSocketAsio.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/*_RESand source-lookup traffic.Fix
m_endpointallocation into each constructor's member-init list so the class invariantm_endpoint != nullptrholds from construction onwards.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/deletepair from the UDP receive hot path.Test plan
-DBUILD_DAEMON=YES -DBUILD_AMULECMD=YES), clean compileamuled, Kad bootstrap from 160 saved contacts, server connection attempts (each goes through the constructor + operator= path), graceful shutdown viaamulecmd … Shutdown, no crashRefs #712.