Skip to content

fix: don't UB if EncryptedStreamSocket Negotiate() is entered with no expected bytes - #779

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/encstream-negotiate-precondition
Jun 1, 2026
Merged

fix: don't UB if EncryptedStreamSocket Negotiate() is entered with no expected bytes#779
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/encstream-negotiate-precondition

Conversation

@got3nks

@got3nks got3nks commented May 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #778. CEncryptedStreamSocket::Negotiate() opens with wxASSERT(m_nReceiveBytesWanted > 0) — a precondition that the negotiation state machine still expects more handshake bytes. mifritscher2 hit it when switching ed2k servers: the kernel still had buffered data for the half-negotiated connection, Read() dispatched into Negotiate() while the counter was already 0, and the debug build aborted.

The asserted precondition is load-bearing — the while loop below assumes a positive count and the math is bogus otherwise. wxASSERT is a no-op in release, so production was undefined behaviour rather than a clean abort. Same shape as the wxASSERT-as-precondition cluster the baseline config in #770 and #772 already converted — just on a site #772 didn't reach.

Fix

Switch the wxASSERT to wxCHECK_MSG returning -1. Both call sites in Read() already treat (uint32_t)(-1) as the encryption-read-error sentinel and bail out cleanly (lines 249-260 and 298-301). Debug builds keep aborting on first mis-use; release builds now drop the connection instead of UB-ing forward.

Out of scope

The deeper question of why Negotiate() is reachable with m_nReceiveBytesWanted == 0 in the first place — i.e. whether the server-switch teardown path should drain the kernel buffer before going to ECS_NEGOTIATING or whether the state machine itself has a missing transition — is left for a follow-up. The negotiation state machine is delicate enough that I'd want a reproducer before changing transition logic; this PR makes the production path safe in the meantime.

Test plan

  • amule + amuled build clean on macOS
  • No behavioural change on the happy path — wxCHECK_MSG short-circuits identically to wxASSERT when the precondition holds
  • Field verification by @mifritscher2 once the build lands

Same pattern as the Logger / UserEvents fixes that landed in #772.

@got3nks
fix: don't UB if Negotiate() is entered with no expected bytes (amule…
b5059c9 
…-project#778)

mifritscher2 hit a debug-build assert switching ed2k servers:

  /home/x/code/amule/src/EncryptedStreamSocket.cpp(437): assert
    "m_nReceiveBytesWanted > 0" failed in Negotiate().

  bt: CServerSocket::OnReceive -> CEMSocket::OnReceive
   -> CEncryptedStreamSocket::Read -> Negotiate -> wxASSERT

CEncryptedStreamSocket::Read() unconditionally calls Negotiate()
whenever the socket sits in ECS_NEGOTIATING and bytes arrive. The
negotiation state machine consumes bytes in chunks of
m_nReceiveBytesWanted; when that counter hits 0 the inner loop exits
and the function returns either via ONS_COMPLETE (advancing to
ECS_ENCRYPTING) or via a state transition that resets the counter to
a fresh positive expectation.

There is a window where the socket can stay in ECS_NEGOTIATING with
m_nReceiveBytesWanted == 0 -- the obvious trigger is the user
switching servers while the kernel still has buffered bytes for the
previous half-negotiated connection. The next OnReceive lands in
Read(), Read() dispatches to Negotiate(), and we hit the precondition
assert.

The asserted precondition is a real bug shape: walking into the while
loop with m_nReceiveBytesWanted == 0 lets std::min(nLen - nRead, 0)
return 0 forever (infinite loop bails via ONS_COMPLETE check, but the
state math is bogus). wxASSERT compiles to a no-op in release builds,
so the production path is undefined behaviour rather than a clean
abort. Same shape as the wxASSERT-as-precondition cluster the
clang-tidy baseline (amule-project#770) and amule-project#772 already converted to wxCHECK_*
elsewhere -- just on a site amule-project#772 did not reach.

Switch this site to wxCHECK_MSG with a -1 sentinel return. Both
callers in Read() already check for nRead == (uint32_t)(-1) and
short-circuit to "encryption read error" -> abort the connection.
Debug builds still abort (wxCHECK_MSG asserts in debug); release
builds now drop the connection cleanly instead of UB'ing forward.
This was referenced May 31, 2026
Closed
Merged
@mifritscher2

mifritscher2 commented May 31, 2026

Copy link
Copy Markdown

I applied that patch - can last some days/week until I hit this again...

@mrjimenez

mrjimenez commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Should we merge or we wait?

@got3nks

got3nks commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

I think it's safe to merge it

@mrjimenez
mrjimenez merged commit c8240f2 into amule-project:master Jun 1, 2026
7 checks passed
@got3nks
got3nks deleted the fix/encstream-negotiate-precondition branch June 3, 2026 14:16
@got3nks got3nks mentioned this pull request Jun 4, 2026
Merged
1 task
got3nks added a commit to got3nks/amule that referenced this pull request Jun 4, 2026
@got3nks
docs(changelog): cover post-amule-project#747 wave + extend flat PR i…
9397f01 
…ndex

Adds 55+ merged PRs to the 3.0.0 changelog since the last update
(amule-project#747, 2026-05-27). Narrative additions cover:

- Packaging: expanded the top list to include the macOS per-arch .app
  bundles and the Windows NSIS installer alongside the existing
  AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785
  (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg
  amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796
  (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths).

- Bug Fixes & Stability: post-amule-project#744 fixes including EC notification
  leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728),
  amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760),
  PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788,
  amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756),
  Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833,
  amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774).

- Translations: late-cycle wave covering French/Turkish manpages
  (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR
  (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for
  date+version drift (amule-project#802).

- Contributors: added ngosang for UX feedback on the late-3.0
  cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the
  user-facing manual at amule-org.github.io.

- Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.
got3nks added a commit to got3nks/amule that referenced this pull request Jun 4, 2026
@got3nks
docs(changelog): cover post-amule-project#747 wave + extend flat PR i…
de8dbcb 
…ndex

Adds 55+ merged PRs to the 3.0.0 changelog since the last update
(amule-project#747, 2026-05-27). Narrative additions cover:

- Packaging: expanded the top list to include the macOS per-arch .app
  bundles and the Windows NSIS installer alongside the existing
  AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785
  (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg
  amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796
  (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths).

- Bug Fixes & Stability: post-amule-project#744 fixes including EC notification
  leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728),
  amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760),
  PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788,
  amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756),
  Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833,
  amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774).

- Translations: late-cycle wave covering French/Turkish manpages
  (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR
  (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for
  date+version drift (amule-project#802).

- Contributors: added ngosang for UX feedback on the late-3.0
  cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the
  user-facing manual at amule-org.github.io.

- Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.
mrjimenez pushed a commit that referenced this pull request Jun 4, 2026
@got3nks @mrjimenez
docs(changelog): cover post-#747 wave + extend flat PR index
f1c3bc0 
Adds 55+ merged PRs to the 3.0.0 changelog since the last update
(#747, 2026-05-27). Narrative additions cover:

- Packaging: expanded the top list to include the macOS per-arch .app
  bundles and the Windows NSIS installer alongside the existing
  AppImage / Flatpak / .dmg / .zip entries. New bullets for #785
  (alc/alcc/cas/wxcas everywhere + Windows amuleweb), #794 (.dmg
  amuleweb path), #789 (<OS>-<arch> artifact naming), #780 / #796
  (Windows DPI + comctl32 manifest), #784 (FHS share/amule paths).

- Bug Fixes & Stability: post-#744 fixes including EC notification
  leak (#797), big-library scaling (#736, #840 superseding #728),
  amulegui ghost entries (#810, #819, #841, #824, #830, #760),
  PartFile early hash (#762), server protocol fixes (#835, #788,
  #721, #787), crypto stream UB (#779), UAF prevention (#756),
  Kad rotation (#795, #799/#805), GTK warning silencing (#833,
  #826/#836), and the clang-tidy worklist (#770, #772-#774).

- Translations: late-cycle wave covering French/Turkish manpages
  (#753/#754/#776), Galician (#763), Slovenian (#771), pt-BR
  (#768/#775/#812), French (#811), plus man-page tooling for
  date+version drift (#802).

- Contributors: added ngosang for UX feedback on the late-3.0
  cycle (#817/#818/#821/#828/#844) and ongoing work on the
  user-facing manual at amule-org.github.io.

- Merged PRs flat index: extended with #746-#845 + #841.
got3nks added a commit to got3nks/amule that referenced this pull request Aug 3, 2026
@got3nks
fix(gui): log the splash timing under a real debug category (amule-pr…
678d012 
…oject#779)

AddDebugLogLineN passes its category straight to CLogger::IsEnabled, whose
index check rejects anything below zero and hits wxFAIL. logStandard is the
-1 sentinel meaning "not a debug category" -- what AddLogLineN passes -- so
asking whether it is enabled is invalid by construction.

The splash always finishes, so this fired on every Debug monolithic launch,
and assertions block by default: the GUI could not start without dismissing
a dialog first. Release builds compile AddDebugLogLineN away entirely and
were unaffected.

Use logGeneral, keeping the line at debug level as intended.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

assert on EncryptedStreamSocket.cpp(437): assert "m_nReceiveBytesWanted > 0" failed in Negotiate().

3 participants

@got3nks @mifritscher2 @mrjimenez