tests: fix wolfSSHd harness bugs that hid skipped and failed tests - #1174
Open
ejohnstown wants to merge 4 commits into
Open
tests: fix wolfSSHd harness bugs that hid skipped and failed tests#1174ejohnstown wants to merge 4 commits into
ejohnstown wants to merge 4 commits into
Conversation
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
August 18, 2026 17:14
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1174
No scan targets match the changed files in this PR. Review skipped.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple wolfSSHd test-harness issues that could mask skipped/aborted tests or misattribute failures to the daemon instead of the client, improving the reliability of apps/wolfsshd/test/ CI signaling without changing daemon/library code.
Changes:
- Hardened the test runner summary logic (RUN_COMPLETE sentinel) and fixed StrictModes log counting to avoid arithmetic-expansion aborts being misreported as a pass.
- Corrected multiple tests’ “return to start dir” behavior by saving the initial directory in
TESTDIRinstead ofPWD. - Fixed privilege-drop negative testing by anchoring key paths to the repository root and preserving client output logs for diagnosis.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/wolfsshd/test/start_sshd.sh | Prevents set -e callers from aborting when PID grep finds no matches. |
| apps/wolfsshd/test/sshd_x509_upn_fail.sh | Uses TESTDIR to restore working directory correctly before log counting. |
| apps/wolfsshd/test/sshd_x509_test.sh | Uses TESTDIR instead of PWD for reliable directory restoration. |
| apps/wolfsshd/test/sshd_scp_fail.sh | Uses TESTDIR instead of PWD for reliable directory restoration. |
| apps/wolfsshd/test/sshd_privdrop_fail_test.sh | Fixes client key/path resolution and retains client logs to disambiguate failures. |
| apps/wolfsshd/test/sshd_large_sftp_test.sh | Uses TESTDIR instead of PWD for reliable directory restoration. |
| apps/wolfsshd/test/sshd_forcedcmd_test.sh | Adds EXIT trap cleanup and fixes path handling by using TESTDIR. |
| apps/wolfsshd/test/sshd_exec_test.sh | Uses TESTDIR instead of PWD for reliable directory restoration. |
| apps/wolfsshd/test/sshd_bad_sftp_test.sh | Uses TESTDIR instead of PWD for reliable directory restoration. |
| apps/wolfsshd/test/run_all_sshd_tests.sh | Adds RUN_COMPLETE guard, fixes StrictModes counting, and tightens teardown pkill matching. |
| apps/wolfsshd/test/error_return.sh | Uses TESTDIR instead of PWD for reliable directory restoration. |
| .gitignore | Ignores per-run privdrop client logs. |
Suppressed comments (1)
apps/wolfsshd/test/start_sshd.sh:95
- Same PID-parsing concern as above:
ps -e | grep wolfsshd | grep -oE "[0-9]+"extracts TIME digits as well as the PID, soNEW_PIDcan contain non-PID values. That undermines the diff-based PID selection and can lead to stopping the wrong process.
Recommend using a PID-only query (pgrep -x wolfsshd, or ps ... | awk ...) consistently for both the pre-start and post-start PID snapshots.
# Set the PID of started sshd. The daemon can still die after sudo returns,
# and grep exits 1 on no match, so guard this the same way and let the
# caller's empty-PID check report it.
NEW_PID=`ps -e | grep wolfsshd | grep -oE "[0-9]+"` || true
PID=`diff <(echo "$CURRENT_PIDS") <(echo "$NEW_PID") | grep '>' | grep -oE "[0-9]+" | head -n1`
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ejohnstown
force-pushed
the
cleanup-aisle-5
branch
2 times, most recently
from
August 18, 2026 21:30
65059b5 to
544e493
Compare
padelsbach
reviewed
Aug 18, 2026
|
|
||
| cd $PWD | ||
| cd "$TESTDIR" | ||
| stop_wolfsshd |
Contributor
There was a problem hiding this comment.
AI says the sudo kill $PID in stop_wolfsshd will bypass cleanup() and return 1 instead of zero. Do we need to guard the kill?
Contributor
Author
There was a problem hiding this comment.
Yes. cleanup() did run -- it just killed the dead pid again. The problem was set -e aborting there before PID was cleared, so the ForceCommand-SFTP scenario was silently skipped. stop_wolfsshd now guards the kill and returns 0.
grep -c prints 0 and exits 1 when nothing matches, so the "|| echo 0" fallback fired too and the count became "0\n0". The arithmetic error unwound bash out of the test block, skipping the last eleven tests while the summary still printed a pass and exited 0. - Use the bare grep -c result and default only the empty case. - Add a RUN_COMPLETE sentinel at the end of each branch that runs tests. - Check the sentinel before the summary so an abort exits non-zero. - Kill lingering daemons by process name, so the teardown does not kill the run itself before that check when invoked by a path holding "wolfsshd".
Eight scripts saved their starting directory in PWD, which the shell rewrites on every cd, so the "cd $PWD" restore landed in the repository root. sshd_forcedcmd_test.sh's second scenario and the log count in sshd_x509_upn_fail.sh silently never did what they cover. - Save the starting directory in TESTDIR, as sshd_pubkey_reject_test.sh does. - Quote "$TESTDIR" at every cd, now that the saved value is really used. - Stop the daemon from a trap in sshd_forcedcmd_test.sh, so its now reachable second scenario cannot leave one on the shared port when set -e aborts. - Take start_wolfsshd's before and after daemon PID snapshots from pgrep -x instead of scraping every digit run out of "ps -e", which mixed the TIME field's clock digits in with the PID and, with a leftover daemon running, stopped the wrong process. - Let both snapshot pipelines fail, so a set -e caller survives no daemon being up and a daemon that dies after sudo returns is reported by the caller's own empty-PID check.
sshd_privdrop_fail_test.sh runs from apps/wolfsshd/test and handed the example clients relative key paths, but the clients call ChangeToWolfSshRoot() before parsing arguments. Every client died at "Error setting private key" and the test blamed the privilege drop. - Anchor the key, payload and client paths at the script's own directory. - Rename the saved directory to TESTDIR so a later cd cannot clobber it. - Report "no fork at all" separately in the timeout diagnostic. - Print the client's own output at every failure exit, so a client that never connects cannot be read as a daemon fault. - Keep the client logs like log.txt, gitignored and removed on success.
ejohnstown
force-pushed
the
cleanup-aisle-5
branch
from
August 20, 2026 00:07
544e493 to
1547084
Compare
stop_wolfsshd killed $PID unconditionally. With the daemon already gone the kill failed, and under "set -e" that aborted the caller -- in sshd_forcedcmd_test.sh before PID was cleared, so the ForceCommand-SFTP scenario was silently skipped, the EXIT trap killed the dead pid a second time, and the script exited 1. - Guard on a non-empty PID, ignore a failed kill and return 0, so the function is safe to call from an EXIT trap. - Clear PID after stopping, so a second call cannot kill a recycled pid. - Remove the temp key dir even when no daemon was recorded, so a daemon that failed to start does not leak it. - Collapse sshd_forcedcmd_test.sh's cleanup() wrapper to a bare trap stop_wolfsshd EXIT now that the function guards itself. - Check the cd back to the test directory in sshd_x509_upn_fail.sh; the log it counts after the client run is the one there.
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.
Three bugs in the wolfSSHd test harness let the suite report a pass while silently skipping tests or blaming the daemon for a broken client invocation. All changes are under
apps/wolfsshd/test/, plus one.gitignoreline; no library or app code is touched.StrictModes count aborting the suite
grep -cprints 0 and exits 1 on no match, so the|| echo 0fallback made the count "0\n0"; the arithmetic error unwound bash out of the test block, skipping the last eleven tests while the summary still passed.RUN_COMPLETEsentinel checked before the summary, and matched the teardownpkillon the process name so it cannot kill the run.$PWDused as a saved directoryPWD, which the shell rewrites on everycd, sosshd_forcedcmd_test.sh's second scenario and thesshd_x509_upn_fail.shlog count never ran as written; save it inTESTDIRinstead.sshd_forcedcmd_test.sh, and letstart_wolfsshd'sps | greppipelines fail underset -e.Privdrop test's client key paths
ChangeToWolfSshRoot()before parsing arguments, so the relative key paths failed and every client died at "Error setting private key" -- read as a privilege-drop failure. Anchor the paths at the script's own directory.