fix: two memory leaks on malformed-packet exception paths (#884, #885) - #886
Merged
mrjimenez merged 2 commits intoJun 6, 2026
Merged
Conversation
CreateSearchExpressionTree builds an SSearchTerm tree from a KADEMLIA2_SEARCH_KEY_REQ payload via recursive descent over a CMemFile. The three boolean branches (AND/OR/NOT) allocate the parent node with a bare new, then make a first recursive call for the left child and a second for the right. The recursive reads (bio.ReadUInt8, ReadString, ReadUInt32, ReadUInt64) throw CEOFException on a truncated payload. With no try/catch in the parser, no handler in Process2SearchKeyRequest, and ~SSearchTerm only freeing astr/tag (not recursing into left/right -- that lives exclusively in the manual Free walk), the parent node and any already-built left subtree leak through stack unwinding all the way up to CClientUDPSocket::OnPacketReceived's catch block. KADEMLIA2_SEARCH_KEY_REQ is wire-reachable pre-auth on the Kad UDP listener (PacketTracking rate-limits per-IP, but does not eliminate slow-burn or multi-source attacks), so the leak is remotely triggerable resource exhaustion. Wrap each boolean-node body in a try/catch that calls the static Free() helper on the partial subtree before re-raising. Free is null-safe and recursively releases the partially built left subtree along with the parent node, restoring the leak-free behaviour of the success/NULL-return paths. The leaf branches (String/MetaTag/Numeric) don't need a guard because the throwing reads in each happen before any node is allocated; their nodes are only at risk when they are the already-built left child of a boolean whose right recursion throws, which the new boolean-branch guards now handle. Fixes amule-project#884.
The OP_SERVERLIST handler heap-allocated its CMemFile wrapper with a bare new and only freed it via an unguarded delete at the end of the case body. The very first ReadUInt8 on an empty payload, and the ReadUInt32/ReadUInt16 calls inside the entry loop on a truncated payload, throw CEOFException; the function-level catch at the end of ProcessPacket logs the bogus packet but does not reach the still-owning local pointer. The CMemFile attach constructor sets m_delete = false, so the underlying wire buffer isn't lost -- only the small wrapper -- but a malicious server can repeat the malformed packet indefinitely. Switch to a stack-allocated CMemFile, which is destroyed on every exit path including the throw, and drop the now-redundant explicit delete. Fixes amule-project#885.
got3nks
added a commit
to got3nks/amule
that referenced
this pull request
Jun 7, 2026
…ule-project#912) Extends existing categories (preferring extensions over new lines): - Performance/Upload: amule-project#898 SlotAllocation default raised. - Networking & Discovery: wire-parser hardening list extended with amule-project#879/amule-project#882/amule-project#890/amule-project#886; new amuleweb security hardening bullet consolidating ngosang's amule-project#869-amule-project#874 triage (all landed in amule-project#875); amulegui list extended with amule-project#857; shared-folder watcher extended with amule-project#858. - Packaging: Windows installer i18n line extended with amule-project#899. - Internals & Refactoring: new docs-polish + code-quality bullets covering amule-project#851/amule-project#855/amule-project#862/amule-project#888/amule-project#900/amule-project#866/amule-project#867/amule-project#895 and amule-project#909/amule-project#910/amule-project#912. - Translations: new pre-release final-wave bullet covering amule-project#847/amule-project#856/ amule-project#891/amule-project#908/amule-project#860/amule-project#904/amule-project#859/amule-project#863/amule-project#861/amule-project#880/amule-project#911/amule-project#901/amule-project#902/amule-project#889/amule-project#868/amule-project#853. - Bug Fixes & Stability: amule-project#850/amule-project#854/amule-project#878/amule-project#906. - CI: ccache wiring (amule-project#892, amule-project#903) + CodeQL binutils-dev (amule-project#907). Contributors footer gains mifritscher and nguyenhoangminhhieu2004-gif (both first-time contributors). PR index extended through amule-project#912.
mrjimenez
pushed a commit
that referenced
this pull request
Jun 8, 2026
Extends existing categories (preferring extensions over new lines): - Performance/Upload: #898 SlotAllocation default raised. - Networking & Discovery: wire-parser hardening list extended with #879/#882/#890/#886; new amuleweb security hardening bullet consolidating ngosang's #869-#874 triage (all landed in #875); amulegui list extended with #857; shared-folder watcher extended with #858. - Packaging: Windows installer i18n line extended with #899. - Internals & Refactoring: new docs-polish + code-quality bullets covering #851/#855/#862/#888/#900/#866/#867/#895 and #909/#910/#912. - Translations: new pre-release final-wave bullet covering #847/#856/ #891/#908/#860/#904/#859/#863/#861/#880/#911/#901/#902/#889/#868/#853. - Bug Fixes & Stability: #850/#854/#878/#906. - CI: ccache wiring (#892, #903) + CodeQL binutils-dev (#907). Contributors footer gains mifritscher and nguyenhoangminhhieu2004-gif (both first-time contributors). PR index extended through #912.
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
2 participants
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
Two independent CWE-401 leaks, both triggered by
CEOFExceptionthrown from a truncated wire payload while raw-pointer state is in flight, both filed today by @ngosang. One commit per fix.Commits
13ff9c1acCreateSearchExpressionTreewithtry/catch. On exception the staticFree()helper is invoked on the partial subtree (it's null-safe and recursively releasesleft/right), then the exception re-raises. Restores the leak-free behaviour of the success / NULL-return paths. The leaf branches (String / MetaTag / Numeric) don't need a guard because the throwing reads in each happen before any node is allocated — those nodes are only at risk when they are the left child of a boolean whose right recursion throws, which the new boolean-branch guards now handle.KademliaUDPListener.cppc98fd28a7new CMemFile(packet, size)in theOP_SERVERLISThandler with a stack object. Destruction now happens on every exit path, including the throw fromReadUInt8/ReadUInt32/ReadUInt16on an empty or truncated server-supplied payload. Drops the now-redundant explicitdelete servers.ServerSocket.cppA small note on the #884 issue text: it states that
CKademlia::ProcessPacket"logs and continues" — actually that dispatcher logs and re-throws in both catch arms (Kademlia.cpp:309 & 312); the final swallower is ClientUDPSocket.cpp:129-135. The leak is real either way — it happens during stack unwinding throughCreateSearchExpressionTree, not at the eventual catch site — so the fix location is unchanged.Test plan
Built clean on macOS local (Apple Silicon, Homebrew) at each commit; daemon links and runs.
#885 — runtime-verified on Ubuntu ARM64 via synthetic harness
The fix is reachable only when amuled is connected to a server that sends a malformed
OP_SERVERLIST. Public eD2k servers don't reliably send OP_SERVERLIST at all (most never reply to OP_GETSERVERLIST), so a synthetic harness was built to drive the path end-to-end against amuled #886:ReadUInt8()to throwCEOFExceptionat the first read inside the handler.amulecmd add ed2k://|server|<IP>|14661|/+connect <IP>:14661. (RequiresFilterLanIPs=0andIpFilterServers=0temporarily — restored afterwards.)~/.aMule/logfile:Servers: Server: OP_SERVERLIST→Received 2 new servers→Saving of server-list completed.Then ~20s later:ServerUDP: >> Sending OP__GlobServStatReq ... to server 8.8.8.8:4444— confirms the two injected entries actually landed in the live server list.Server: Packet Received: Prot e3, Opcode 32, Length 0→Servers: Server: OP_SERVERLIST→Bogus packet received from server: SafeIO::EOF: Attempt to read past end of file.Stack unwinding propagates fromReadUInt8out through the case body to the function-level catch — with the stack-allocatedCMemFilefrom this PR, the wrapper is destroyed during unwind (the leak the PR closes).#884 — code review only
CreateSearchExpressionTreeis reached when amuled receives aKADEMLIA2_SEARCH_KEY_REQfrom another Kad peer. The Kad node on the test VM is firewalled, so peers don't issue search-key-reqs at it, and a synthetic harness for that path would require eMule-encrypted UDP + a working DH key agreement — disproportionate effort for a leak fix this surgical. The diff is three mechanicaltry/catchwrappers around AND/OR/NOT bodies with no success-path semantics change; the success path is byte-for-byte identical to before.