ServerSocket: demote OP_IDCHANGE sanity checks to debug logs (#610) - #613
Merged
mrjimenez merged 1 commit intoMay 15, 2026
Merged
Conversation
…roject#610) A misbehaving server (e.g. the "Test_server with nat_traversal v0.6" reported in amule-project#610) can put its own IP in the dwServerReportedIP field instead of the client's, which trips wxFAIL / wxASSERT during the OP_IDCHANGE parse. Both checks are purely diagnostic -- the downstream consumer at line 327 only uses dwServerReportedIP when new_id is LowID, so the bogus HighID value is silently ignored anyway -- but they crash debug builds for users connecting to weird servers. Replace the assertions with AddDebugLogLineN(logServer, ...) lines that record the bogus value for diagnosis without aborting. As mifritscher2 noted, assertions should signal corrupted internal state, not malformed input from a remote peer. Reported by mifritscher2.
This was referenced May 15, 2026
mrjimenez
pushed a commit
that referenced
this pull request
May 15, 2026
CFileDataIO::WriteStringCore wxFAIL_MSGs when asked to serialise a string larger than 0xFFFF bytes with a uint16 length prefix. The intent (per the surrounding comment) is to flag peer-supplied data that expands past 16 bits after ISO8859-1 -> UTF-8 conversion -- exactly the "external data should not trigger an assertion" pattern that PR #613 addressed for OP_IDCHANGE. The immediately-following code already does the right thing: it truncates real_length to 0xFFFF and adjusts sLength accordingly, so release builds silently absorb the oversized input. Debug builds, however, abort on what is harmless input -- crashing amuled on every periodic save where any single Kad contact field has been poisoned (see #246, drzraf 2021 and fekir 2024). Replace wxFAIL_MSG with AddDebugLogLineN(logCFile, ...) that records the original byte count for diagnosis without aborting. The truncation path is unchanged; on-disk format is unchanged. Same shape as PR #613 / issue #610. Reported by drzraf and fekir.
got3nks
added a commit
to got3nks/amule
that referenced
this pull request
Jul 26, 2026
…oject#613) Persist the Local/Global/Kad search type across restarts, and save list column widths synchronously in SaveGUIPrefs() so they survive the hide-on-close / tray-Exit path. Both cross-platform. Addresses amule-project#608 (bugs 2 & 3).
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
A misbehaving server can put its own IP in the
dwServerReportedIPfield of theOP_IDCHANGEreply instead of the client's, which tripswxFAIL/wxASSERTduring the parse atServerSocket.cpp:279,282. mifritscher2 hit this in #610 connecting to "Test_server with nat_traversal v0.6". Per his point, assertions should signal corrupted internal state, not malformed input from a remote peer.Why it's safe to demote
The downstream consumer at
ServerSocket.cpp:327only readsdwServerReportedIPwhennew_idis LowID:So on HighID (the case both asserts target) the bogus value is silently ignored regardless of whether the assert fires. The asserts therefore never guarded any behaviour — they only existed to flag protocol oddity.
Change
Replace
wxFAILandwxASSERT(...)withAddDebugLogLineN(logServer, ...)lines that record the offending value for diagnosis. Behaviour on real servers is unchanged; debug builds no longer abort on contact with non-conforming servers.Closes #610.