chore: add baseline .clang-tidy configuration by got3nks · Pull Request #770 · amule-project/amule

got3nks · 2026-05-30T09:53:09Z

Summary

Curated .clang-tidy baseline that surfaces ~40 bug-shape warnings across the project source — versus 226 with the clang-tidy default analyzer set, 1669 with broad bugprone-* / cppcoreguidelines-* enablement, or 7466 with the noisiest checks added.

Catches every actionable finding the linter currently reaches (the wxASSERT-as-precondition cluster in Logger / UserEvents / MD4Hash, RLE.cpp NULL pointer paths, BitVector::SetBuffer precondition, html.c fopen-without-NULL-check, ThreadTasks enum-cast-out-of-range, TextClient uninitialised field, ED2KLinkParser getenv-NULL, a PartFile uint16/uint32 loop overflow, a kademlia misplaced time_t cast, an aLinkCreator ed2khash.cpp user-cancel leak, a few dead stores) while suppressing the known noise classes (the safe nstrdup in StringFunctions.h, wxThread detached-mode "leaks", wx-takes-ownership sinks, the C11 *_s portability complaints, the wx 3.2 virtual-call-in-dtor false positives, cppcoreguidelines stylistic noise).

Full rationale per-check is in the file's comment header.

Layout

Root .clang-tidy — curated check list + documentation
src/extern/.clang-tidy — Checks: '-*' (vendored wx code)
src/webserver/src/.clang-tidy — Checks: '-*' (PHP interpreter, mostly generated)

Three flex/bison-generated files live alongside hand-written source in src/ root (Scanner.cpp, Parser.cpp, IPFilterScanner.cpp) and can't be directory-excluded; they're documented in the root comment block as a known caveat — patching them with NOLINT would either get clobbered by regeneration or churn the checked-in output.

Validation

Tested against current master tip on Linux ARM64 (clang-tidy-21). Output is 125 warnings total — ~40 bug-shape, ~60 low-priority performance, ~24 in the documented generated files. Every actionable warning maps to either a known issue or a real bug we want to address in follow-up cleanup PRs.

No CI gating

WarningsAsErrors: '' — informational baseline only. Nothing in CI fails on this. Intended as a worklist to chip away at, not as a quality gate.

Curated check set that surfaces ~40 bug-shape warnings across the project (vs. 226 with the analyzer defaults, 1669 with broad bugprone-* / cppcoreguidelines-* enablement) while suppressing the known noise classes: - clang-analyzer-security.insecureAPI.strcpy 119 hits from one safe nstrdup call site (libs/common/StringFunctions.h - new char[strlen(src)+1]; strcpy). Bounded by construction; the checker can't see the size relation. - clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling Asks for the C11 *_s variants (vsnprintf_s, fprintf_s, ...). MSVC- only, so portable codebases will never adopt them. - clang-analyzer-cplusplus.NewDeleteLeaks Doesn't model wxThread's detached-mode self-delete (new X; X->Create(); X->Run() - thread frees itself when Entry() returns) or wx-takes-ownership sinks (wxLog::SetActiveTarget(new ...)). All 28 hits are this class. - clang-analyzer-optin.cplusplus.VirtualCall Most fires are most-derived-class dtors (effectively safe) or inside wx 3.2 internal headers we don't control. - cppcoreguidelines-* (entirely) The codebase predates the C++ Core Guidelines by 15+ years; enabling the group dumps ~1100 stylistic warnings that aren't bugs. - bugprone-reserved-identifier Fires on __WINDOWS__ / __DEBUG__ / __TFILE__ and every other legitimate platform-define macro (~2000 noise hits). - performance-enum-size ~5300 hits - every enum that could be smaller. Stylistic. Captures every actionable finding the linter currently reaches: the wxASSERT-as-precondition cluster (Logger.cpp:183, UserEvents.cpp, MD4Hash.h:202), RLE.cpp NULL pointer paths, BitVector::SetBuffer precondition, html.c fopen-without-NULL-check, ThreadTasks.cpp:55 enum-cast-out-of-range, TextClient.cpp:204 uninitialised field, ED2KLinkParser.cpp:107 getenv(NULL), a PartFile.cpp:3241 uint16/uint32 loop overflow, a kademlia/Search.cpp:241 misplaced time_t cast, an aLinkCreator/ed2khash.cpp:114 leak on user-cancel, and a few dead stores. WarningsAsErrors is left empty - this is an informational baseline, not a CI gate. Layout: .clang-tidy - root, curated check set src/extern/.clang-tidy - vendored wx, Checks: -* src/webserver/src/.clang-tidy - PHP interpreter, Checks: -* Three flex/bison-generated files live in src/ root alongside hand- written source (Scanner.cpp, Parser.cpp, IPFilterScanner.cpp) and can't be directory-excluded; they're documented in the root comment block as a known caveat - patching them with NOLINT would either get clobbered on regeneration or churn the checked-in output. Validated against current master on Linux ARM64 (clang-tidy-21): 125 warnings total, ~40 bug-shape, ~60 low-priority performance, ~24 in the documented generated files.

Logger / UserEvents wxASSERT is a no-op in release builds, so the existing "wxASSERT(precond); use_value()" pattern in CLogger::GetDebugCategory and in CUserEvents' s_EventList accessors would read past the array in release if an out-of-range index ever reached them (where in debug it would have asserted + aborted). clang-tidy surfaces all of these as clang-analyzer-security.ArrayBound on the release-build code path. Switch to wxCHECK_MSG / wxCHECK_RET on the same precondition: debug builds still assert + abort on first mis-use, release builds now log the failure and either return a safe sentinel (entry [0] of the array, which is always present and well-defined) or short-circuit the void path. UserEvents.cpp's CheckIndex was previously guarded behind #ifdef __WXDEBUG__ because wxASSERT doesn't evaluate its argument; wxCHECK_* does, so the function is now always-defined. It is a trivial one-liner and adds no measurable cost in release. Out of scope for this commit: the MD4Hash.h:202 warning that appeared in the same cluster on lint output is not actually a wxASSERT issue -- the analyzer can't track the per-byte writes through RawPokeUInt64 in CMD4Hash::SetHash, so it concludes m_hash is partially uninitialised when EncodeSTL reads it. The code is correct; the analyzer is symbolically wrong on that one. Verified the warnings disappear: a re-run of clang-tidy with the .clang-tidy baseline from #770 over the two changed files emits zero ArrayBound hits, down from 12 on the pre-fix tree.

Fixes the medium-priority cluster from the lint worklist that's surfaced by the .clang-tidy baseline in #770. Each is a small, isolated safety guard: - src/utils/cas/html.c:89 -- fopen(template,"r") result wasn't NULL- checked before fgetc() dereferenced it; a missing template file crashed the CGI helper. Add the NULL-check + perror+exit to match the existing calloc/fstat error-handling shape in this file. - src/PartFile.cpp:3341, 3420 -- loop variable promoted from uint16 to uint32 to match `partCount` (declared uint32 at line 3193). GetPartCount() currently returns uint16 so today's behaviour is unchanged, but a future widening wouldn't silently truncate the iteration above 65535. Fires bugprone-too-small-loop-variable. - src/utils/aLinkCreator/src/ed2khash.cpp:114 -- user-cancel path (progress hook returning false) freed `buf` but leaked the realloc()-grown `tmpCharHash`. Hash of every mid-run cancelled file leaked the cumulative parthash buffer. Free both. Fires clang-analyzer-unix.Malloc. - src/kademlia/kademlia/Search.cpp:241 -- `(time_t)(uint32 + 3) > time(NULL)` did the addition in uint32 before the cast, so the comparison would reorder near the 2106 32-bit time wraparound. Cast first, add second. Eighty years out, but cheap to write correctly. Fires bugprone-misplaced-widening-cast. - src/ED2KLinkParser.cpp:107 -- `string(getenv("HOME"))` is UB if HOME is unset (rare but possible). Mirror the existing macOS-branch pattern further up the same function: `string(home ? home : "")`. Fires clang-analyzer-cplusplus.StringChecker. - src/BitVector.h:144 -- SetBuffer() ran memcpy(m_vector, src, m_bytes) unconditionally. After clear(), m_vector is NULL and m_bytes is 0, so the call reduces to memcpy(NULL, src, 0), which is C-standard UB even though real libcs no-op it. Guard on m_bytes (matches SetAllTrue() above). Fires clang-analyzer-core.NonNullParamChecker. All builds clean across amule, amuled, amulegui, cas, alc on macOS. Re-run of clang-tidy with the #770 baseline confirms each of the above sites no longer fires; no new warnings introduced.

Third cleanup pass from the worklist, addressing the items the analyzer flags as latent (not crashy) but worth tidying: - src/libs/ec/cpp/ECTag.h -- EC_IPv4_t::EC_IPv4_t() left m_ip and m_port uninitialised, so a stack-allocated instance built with the default ctor (TextClient.cpp:286 and similar) had garbage in the fields before the caller explicitly assigned them. Add C++11 default member initialisers; the empty ctor body now produces a well-defined zero value. Caught on lint as clang-analyzer-optin.cplusplus.UninitializedObject. - src/ThreadTasks.h + ThreadTasks.cpp -- the EHashes enum had only EH_AICH = 1 and EH_MD4 = 2, but the ctor used (EHashes)(EH_MD4 | EH_AICH), which is value 3, not a member of the enum. Add EH_MD4_AND_AICH = EH_MD4 | EH_AICH as a named member and use it directly, no cast needed. Caught on lint as clang-analyzer-optin.core.EnumCastOutOfRange. - src/PartFileHashThread.cpp -- elapsedMs is computed for a debug log line; AddDebugLogLineN compiles to a no-op in release, so the variable becomes a dead store. wxUnusedVar(elapsedMs) after the macro silences the warning without touching the debug-build output. Caught on lint as clang-analyzer-deadcode.DeadStores. - src/ECSpecialMuleTags.cpp -- the EC_TAG_PREFS_STATISTICS handling block was a placeholder with a //#warning TODO inside and an assignment-form if-statement whose value was overwritten by the next block (real dead store, not a false positive). Drop the assignment, keep the existence check + TODO so future work has an obvious home. Caught on lint as clang-analyzer-deadcode.DeadStores. Out of scope: the RLE.cpp zero-size memset/memcpy warnings on lines 121, 148, 151 (clang-analyzer-core.NonNullParamChecker / uninitialized.Assign / security.ArrayBound). The Decode() function's two-pass realloc dance defeats the analyzer's symbolic execution even after the obvious fixes (allocate at least 1 byte + explicit memset + size guards); the warnings persist because the analyzer follows mathematically-impossible paths through the m_len == 0 branch. A proper fix likely needs an early-return restructure or a switch to std::vector that the analyzer can track better. Deferred. All builds clean across amule, amuled, amulegui, amulecmd on macOS. Re-run of clang-tidy with the #770 baseline confirms each of the four sites no longer fires; no new warnings introduced.

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 (#770) and #772 already converted to wxCHECK_* elsewhere -- just on a site #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.

…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.

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.

…mule-project#770) IDC_EXT_CONN_REQUIRE_ENCRYPTION configures the daemon's own EC listener (read from amuled's config, never remote.conf), so like the other external-connection controls it is meaningless from a remote GUI. It was added without being placed in amuledOnlyPrefs[], so in amulegui it stayed visible while its container box and siblings were hidden -- left orphaned in the collapsed layout, drawn on top of the aMule API server parameters box below it. Add it to the hide list.

got3nks force-pushed the chore/clang-tidy-baseline branch from 9b26c75 to 5155dba Compare May 30, 2026 09:59

This was referenced May 30, 2026

linting issues #766

Closed

cleanup: wxASSERT -> wxCHECK_MSG on array-access guards in Logger / UserEvents #772

Merged

cleanup: 6 safety fixes from clang-tidy worklist #773

Merged

cleanup: 4 cosmetic fixes from clang-tidy worklist #774

Merged

mrjimenez merged commit 4157f85 into amule-project:master May 30, 2026
7 checks passed

This was referenced May 31, 2026

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

Merged

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

Closed

got3nks deleted the chore/clang-tidy-baseline branch June 3, 2026 14:15

got3nks mentioned this pull request Jun 4, 2026

pre-release(3.0.0): refresh CHANGELOG with post-#747 wave + regenerate .po translations #846

Merged

1 task

got3nks mentioned this pull request Jun 9, 2026

How about adding a .clang-format to the project? amule-org/amule#35

Closed

Navigation Menu

Uh oh!

chore: add baseline .clang-tidy configuration - #770

chore: add baseline .clang-tidy configuration#770
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:chore/clang-tidy-baseline

got3nks commented May 30, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

got3nks commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Layout

Validation

No CI gating

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

got3nks commented May 30, 2026 •

edited

Loading