wolfssh client app: -E logging, CI coverage, two build/exit fixes - #1168
Open
ejohnstown wants to merge 10 commits into
Open
wolfssh client app: -E logging, CI coverage, two build/exit fixes#1168ejohnstown wants to merge 10 commits into
ejohnstown wants to merge 10 commits into
Conversation
ejohnstown
requested review from
JacobBarthelmeh and
wolfSSL-Fenrir-bot
and
a lite review from Copilot
August 13, 2026 20:19
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR wires up the wolfSSH client app’s -E logfile flag end-to-end, extends CI to build/test the client app, and fixes a shutdown drain behavior that could incorrectly affect exit status.
Changes:
- Implement
-Eby installing a logging callback that appends to a file and enables logging. - Add a new
scripts/sshclient.testand include it in distribution scripts formake checkcoverage. - Improve configure/CI behavior around
--enable-sshclientwith single-threaded wolfSSL and adjust multi-compiler CI to build the client.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/sshclient.test | Adds an end-to-end client test (remote command, terminal session, -E logging) against echoserver. |
| scripts/include.am | Ships the new client test script in the dist script list. |
| configure.ac | Rejects explicit --enable-sshclient with single-threaded wolfSSL; drops implied sshclient under --enable-all. |
| apps/wolfssh/wolfssh.c | Implements -E logging callback/file lifecycle, makes config parsing happen before init, and adjusts shutdown drain return handling. |
| apps/wolfssh/README.md | Documents threading requirement and clarifies -E behavior. |
| .github/workflows/sshd-test.yml | Renames workflow job to reflect it now covers multiple apps. |
| .github/workflows/multi-compiler.yml | Enables client app build in the multi-compiler workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and removed request for
wolfSSL-Fenrir-bot
August 14, 2026 15:40
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1168
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
- -E was parsed into config.logFile and printed by -G, never read. - Install a logging callback that writes to the named file, following what wolfsshd does for its own -E. - Turn logging on with wolfSSH_Debugging_ON(). Installing the callback is not enough on its own, the file came out empty in any build that wasn't --enable-debug, including the --enable-all builds where the library has all of its logging compiled in. wolfsshd turns logging on the same way. - Match DefaultLoggingCb()'s format, timestamp and level tag, so a log written to the file and one written to stderr are comparable. That function's GetLogStr() is private to the library, so the level names are repeated in the app. - Parse the command line and open the file in main(), before wolfSSH_Init(), so the start up messages land in the file. - Close the file after wolfSSH_Cleanup(). The callback cannot be uninstalled, so it ran with a closed stream and segfaulted on exit. It falls back to stderr. - Name the stream logFileStream, apart from struct config's logFile, which is the path it was opened from. - Drop the always true condition around the session threads.
- --enable-sshclient defaults to no, so the app was built only by the configs that use --enable-all, and never under the multi-compiler warning flags. Add it to the multi-compiler matrix. - Add scripts/sshclient.test, run by make check. It covers the client's sessions and the -E log file against the echoserver. - The script is not gated on BUILD_SSHCLIENT. It exits 77 when the client app or the echoserver isn't there, so every build runs it and the ones without the app report it as a skip. - Check the client and the echoserver by asking each for its usage message, not by looking for the file. Both are libtool wrapper scripts in the build tree, and a wrapper outlives a reconfigure that drops the program it wraps, then runs only far enough to say so. - The echoserver runs in echo mode and the client's stdin comes from a fifo written a piece at a time, so the session carries data and ends on its own. Each client run has a watchdog. - Rename sshd-test.yml's job to cover both apps. That workflow builds the client app along with wolfsshd. - Check that the command reaches the server, now that the client sends it rather than discarding it. - Make the SINGLE_THREADED guard a preprocessor #error. The runtime err_sys() only caught the misconfiguration in an autotools build that got as far as running; the #error catches it at compile time for the IDE and plain Makefile builds too. - Treat WS_WANT_READ and WS_WANT_WRITE out of wolfSSH_worker() as a clean shutdown. The socket is non-blocking, so the peer having nothing ready is not a session failure.
wolfSSH_shutdown() returns WS_WANT_WRITE when the channel EOF, exit and close messages are still queued on the non-blocking socket. Masking that to WS_SUCCESS reported a clean exit for a session whose close messages never reached the peer. Mask a want write from the drain worker only, where the close messages are already sent. The want read masking stays on both, wolfSSH_shutdown() runs a worker of its own and passes that want read back.
The client runs every session's I/O on threads, so it needs a threaded wolfSSL. configure probes for SINGLE_THREADED when the client app is enabled. Asking for the app with --enable-sshclient is an error, getting it from --enable-all drops the app instead, so --enable-all still configures against a single threaded wolfSSL. The compile time check stays for the builds that never run configure. That leaves the SINGLE_THREADED terms in the app's own guards unreachable, so drop them.
A send the socket wasn't ready for stays queued but still reports the data as taken, so the client waited on a reply to a message it never sent. Flush after a queued send, a terminal size change, and at shutdown. The shutdown drain reports its want read as WS_FATAL_ERROR, so read the status with wolfSSH_get_error(); an ordinary shutdown was exiting 1. Time out readPeer()'s select() so a flush can't strand the reader.
The echoserver needs -N under WOLFSSH_TEST_BLOCK, and even with it leaves a failed write queued while it waits on the peer, so a session stalls. scp.test and get-put.test skip the build too.
The paths are built from pwd, so an unquoted use split on a build directory with a space in it, and the cleanup's rm -rf then deleted whatever the first word named. - Quote work_dir and every path derived from it - Pass the directory to rm after -- - Replace the two echo -e calls, dash prints a literal -e
On the MSVC path the input thread is never waited on, it blocks in a console read with nothing to cancel it, so it can still be logging when main closes the file named by -E. - Flush the log there and let process exit close the stream - The POSIX path joins its threads first, it still closes the file
FlushQueuedSend() retried wolfSSH_worker() for as long as it reported WS_WANT_WRITE. A peer that stops reading never lets the socket drain, so the sending thread spun there, and at the shutdown drain that thread was main, leaving the client unable to exit. - Give the retry a ten second deadline - Return the still pending WS_WANT_WRITE to the caller - Take that for done at the shutdown drain, the socket closes next - Mask WS_REKEYING, the worker only reports it once the send is out, and readInput() was taking it for a send failure
bytes held select()'s return in a word32, so a -1 became 0xFFFFFFFF and ran the read path on descriptor sets select() had left alone. The SIGWINCH handler interrupts this select, so a terminal resize reaches it. - Keep the result in an int - Retry on EINTR, report anything else - Same fix readPeer() in examples/client/client.c already carries
ejohnstown
force-pushed
the
client-app-fixes
branch
from
August 14, 2026 21:54
b7be810 to
d5bea3f
Compare
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.
Wire up the client's
-Elog file option-Ewas parsed and printed by-G, never read. Installs a callback that writes the log to the file inDefaultLoggingCb's format.Build and test the client app in CI
scripts/sshclient.test(sessions and-Eagainst the echoserver) tomake check. Exits 77 where the app is not built.--enable-sshclientto the multi-compiler matrix; the app never saw the warning flags.Keep a failed shutdown send out of the exit status
WS_WANT_READ/WS_WANT_WRITEfrom the shutdown drain overrode the remote command's status with exit code 1. The socket is non-blocking; nothing ready is not a failure.Don't build the client app against a single threaded wolfSSL
--enable-allenables the client, so bothsinglethread-checkcells failed to compile. configure now errors on explicit--enable-sshclientand drops the client when it is implied.#errorforuser_settings.hbuilds that skip configure.