tidy clang-tidy#1002
Open
ejohnstown wants to merge 6 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a clang-tidy cleanup pass across wolfSSH, focused on eliminating diagnostics (primarily readability-named-parameter) while also addressing a few correctness/safety findings (signal-handler safety, SFTP time conversion reentrancy, SCP bounds check, and sshd config include recursion).
Changes:
- Add parameter names to many public/internal header prototypes and a few forward declarations, rewrapping long prototypes to match existing continuation style.
- Improve safety/correctness in a few targeted areas: remove redundant preprocessor guarding, avoid logging from a signal handler, use reentrant time conversion for SFTP longname creation, and tighten an SCP timestamp bounds check.
- Bound
Includerecursion inwolfsshdconfiguration loading via a tracked include depth and a maximum depth constant.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/wolfscp.h | Names parameters in SCP API declarations; rewraps long prototypes. |
| wolfssh/ssh.h | Names parameters in core public API declarations and callback setters. |
| wolfssh/port.h | Names parameters for select portability-layer prototypes (stdio + string + pread/pwrite). |
| wolfssh/log.h | Names parameters in the public logging API signature. |
| wolfssh/internal.h | Names parameters in internal APIs (channels, I/O handlers, packet helpers, SCP helpers). |
| wolfssh/agent.h | Names parameters in agent API/internal declarations and rewraps long prototypes. |
| src/wolfsftp.c | Switches SFTP longname time conversion to WLOCALTIME and simplifies preprocessor guards. |
| src/wolfscp.c | Refactors SCP timestamp parsing to avoid inc/dec inside conditions and tightens bounds. |
| src/log.c | Updates default logging callback forward-declaration parameter names/wrapping. |
| src/internal.c | Removes redundant inner #ifdef WOLFSSH_CERTS inside an already-guarded function. |
| examples/sftpclient/sftpclient.c | Annotates wolfSSH_SFTP_Interrupt() call in signal handler with NOLINT. |
| examples/scpclient/scpclient.h | Names the thread entry parameter in the example header prototype. |
| apps/wolfsshd/wolfsshd.c | Removes fprintf from signal handler; logs shutdown message from main loop. |
| apps/wolfsshd/test/test_configuration.c | Names the fmt parameter in the test logger prototype. |
| apps/wolfsshd/configuration.c | Adds include-depth tracking + maximum recursion bound; annotates bounded recursion with NOLINTNEXTLINE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Clean up clang-tidy readability-named-parameter findings across wolfSSH headers and sources.
Cleanup clang-tidy readability-redundant-preprocessor finding.
Cleanup clang-tidy bugprone-signal-handler findings. - wolfsshd: drop fprintf from interruptCatch; the main accept loop logs "Closing down wolfSSHD" after seeing quit set. - sftpclient: annotate wolfSSH_SFTP_Interrupt call in sig_handler with NOLINT, since the function is only a single byte store and is safe to call from a signal handler.
Cleanup clang-tidy concurrency-mt-unsafe finding. - SFTP_CreateLongName: switch from XGMTIME (gmtime) to WLOCALTIME, which expands to gmtime_r/localtime_r/ localtime_s per platform.
Cleanup clang-tidy bugprone-inc-dec-in-conditions finding. - ParseTimestamp: move ++idx out of the || conditions and tighten the bound from > bufSz to >= bufSz.
Cleanup clang-tidy misc-no-recursion finding. - wolfSSHD_ConfigLoad: track depth on WOLFSSHD_CONFIG and reject loads past WOLFSSHD_MAX_INCLUDE_DEPTH (16). - HandleInclude, HandleConfigOption, ParseConfigLine, wolfSSHD_ConfigLoad: annotate the call cycle with NOLINTNEXTLINE pointing at the bound. - Add a recursive configuration test.
padelsbach
reviewed
Jun 4, 2026
|
|
||
| /* create new configure for altered options specific to the match */ | ||
| if (ret == WS_SUCCESS) { | ||
| newConf = wolfSSHD_ConfigCopy(*conf); |
Contributor
There was a problem hiding this comment.
AI tells me wolfSSHD_ConfigCopy does not copy the includeDepth param, and thus could be wiped out here, partially defeating the purpose of this commit. Should we copy that field?
padelsbach
reviewed
Jun 4, 2026
| * wrapper so the shared static buffer behind gmtime() is | ||
| * not used. */ | ||
| mtime = (time_t)atr->mtime; | ||
| if (!WLOCALTIME(&mtime, &localTime)) { |
Contributor
There was a problem hiding this comment.
This switches from UTC time to local time in all platforms except Zephyr which uses gmtime_r (in port.h). Is this ok?
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.
Clean up clang-tidy findings across wolfSSH. The checks set initially flagged 378 issues across six categories; this branch clears all of them. Build is clean (exit 0); no functional or wire-protocol changes.