[wip] merge - #842
Open
Tess Gauthier (tgauth) wants to merge 1168 commits into
Open
Conversation
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR looks like an upstream merge/work-in-progress that (a) refreshes some core OpenSSH sources/docs, (b) extends the unit-test harness to support benchmarking mode, and (c) adds a set of GitHub tooling/scripts and merge/test instructions to support the Windows fork’s upstream-merge workflow.
Changes:
- Add benchmarking mode plumbing to the unit-test helper + propagate
UNITTEST_ARGSthrough unittest makefiles andregress/Makefile. - Refactor known_hosts entry writing in
hostfile.cto format into ansshbufand write in onefwrite(), plus stricter parsing of truncated lines. - Add multiple
.github/tools/*.ps1“merge/build/test” helper scripts and extensive merge/testing documentation.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ssh-agent.c | Casts signalled_keydrop for logging format correctness; version bump. |
| scp.1 | Updates option docs for -3 / -R behavior; date/version bump. |
| regress/unittests/win32compat/tests.c | Adds benchmarks() entrypoint (currently delegates to tests()). |
| regress/unittests/utf8/tests.c | Adds benchmarks() stub (“no benchmarks”). |
| regress/unittests/utf8/Makefile | Expands linked sources and passes ${UNITTEST_ARGS} into the test runner. |
| regress/unittests/test_helper/test_helper.h | Adds benchmark macros + new bench_* API declarations and test_is_benchmark(). |
| regress/unittests/test_helper/test_helper.c | Implements benchmark mode CLI flags and timing/statistics collection. |
| regress/unittests/sshsig/tests.c | Adds benchmarks() stub (“no benchmarks”). |
| regress/unittests/sshkey/tests.c | Routes benchmark mode to sshkey_benchmarks(). |
| regress/unittests/sshkey/test_sshkey.c | Adds keygen/sign/verify benchmarks using new BENCH macros. |
| regress/unittests/sshbuf/tests.c | Adds includes + benchmarks() stub (“no benchmarks”). |
| regress/unittests/misc/tests.c | Adds benchmarks() stub (“no benchmarks”). |
| regress/unittests/misc/Makefile | Passes ${UNITTEST_ARGS} into the test runner. |
| regress/unittests/match/tests.c | Adds benchmarks() stub (“no benchmarks”). |
| regress/unittests/match/Makefile | Passes ${UNITTEST_ARGS} into the test runner. |
| regress/unittests/kex/tests.c | Adds benchmarks() entrypoint (delegates to kex_tests()). |
| regress/unittests/kex/test_kex.c | Adds benchmark-aware KEX path and allows cipher/mac overrides in proposals. |
| regress/unittests/kex/Makefile | Passes ${UNITTEST_ARGS} into the test runner. |
| regress/unittests/hostkeys/tests.c | Adds includes + benchmarks() stub (“no benchmarks”). |
| regress/unittests/hostkeys/Makefile | Passes ${UNITTEST_ARGS} into the test runner (while preserving -d testdata). |
| regress/unittests/conversion/tests.c | Adds benchmarks() stub (“no benchmarks”). |
| regress/unittests/conversion/Makefile | Passes ${UNITTEST_ARGS} into the test runner. |
| regress/unittests/bitmap/tests.c | Scales test iteration count with fast/slow modes + adds benchmarks() stub. |
| regress/unittests/bitmap/Makefile | Expands linked sources and passes ${UNITTEST_ARGS} into the test runner. |
| regress/unittests/authopt/tests.c | Adds benchmarks() stub (“no benchmarks”). |
| regress/unittests/authopt/Makefile | Passes ${UNITTEST_ARGS} into the test runner (while preserving -d testdata). |
| regress/unittests/Makefile.inc | Adds -lm, introduces UNITTEST_BENCH* vars, and builds UNITTEST_ARGS incl. -b/-B/-O. |
| regress/Makefile | Adds unit-bench target and threads benchmark/test args through unit test invocations. |
| hostfile.c | Refactors host entry formatting/writing; adds truncated-line handling while parsing hostkeys. |
| defines.h | Adds fallback INFINITY definition when missing. |
| configure.ac | Detects sqrt()/-lm for unit tests and checks for INFINITY declarations. |
| Makefile.in | Appends configured @TESTLIBS@ and adds unit-bench wrapper target. |
| .github/tools/Test-OpenSSHFunctionality.ps1 | Adds Windows E2E functional SSH test script (install service, create user, connect, cleanup). |
| .github/tools/Test-OpenSSHBuild.ps1 | Adds build artifact verification + build log parsing helper. |
| .github/tools/Test-MergePrerequisites.ps1 | Adds pre-merge environment validation (tools, remotes, target tag, clean state). |
| .github/tools/Start-OpenSSHBuild.ps1 | Adds build wrapper around OpenSSHBuildHelper with log-marker based success detection. |
| .github/tools/Save-MergeResolution.ps1 | Adds merge-conflict resolution recorder into .git/merge-resolution-log.json. |
| .github/tools/Replay-MergeResolutions.ps1 | Adds conflict-resolution replay tool that writes/stages saved resolutions. |
| .github/tools/Invoke-OpenSSHTests.ps1 | Adds orchestrator to run Unit/Bash/E2E tests via OpenSSHTestHelper on Windows. |
| .github/tools/Invoke-Git.ps1 | Adds structured git operation wrapper (merge/cherry-pick/status/log/diff/etc.) with async I/O. |
| .github/tools/Get-ConflictContext.ps1 | Adds 3-way conflict context extractor for difficult merges. |
| .github/tools/Get-CommitGroups.ps1 | Adds GitHub API-based upstream commit batching tool (CI success or CI presence). |
| .github/setup_ci.sh | Adjusts Cygwin ACL handling and changes Cygwin setup.exe drive path. |
| .github/prompt/merge.prompt.md | Adds merge prompt for agents describing the two-phase merge workflow and tool usage. |
| .github/instructions/testing.instructions.md | Adds comprehensive testing instructions for agents (incl. new tools). |
| .github/instructions/setup.instructions.md | Adds repo setup instructions for agents. |
| .github/instructions/repository-overview.instructions.md | Adds repo structure + Windows compat layer overview. |
| .github/instructions/merge/research.instructions.md | Adds merge research references and heuristics. |
| .github/instructions/merge/merge-process-overview.instructions.md | Adds end-to-end merge workflow instructions (scratch/real branch phases, tooling). |
| .github/instructions/merge/agent-communication-merge.instructions.md | Adds required communication templates for tool output and merge progress. |
| .github/instructions/getting-started.instructions.md | Adds index/entrypoint doc linking to key instructions. |
| .github/instructions/build.instructions.md | Adds build instructions and policies for agents (warnings baseline, paths.targets restore, etc.). |
| .github/instructions/agent-communication.instructions.md | Adds general communication rules for agents (chat vs tool output). |
| .github/ci-status.md | Adds CI badge section for OpenSSH 10.0 branch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+696
to
+699
| free(bench_samples); | ||
| bench_nalloc = BENCH_SAMPLES_ALLOC; | ||
| bench_samples = xcalloc(sizeof(*bench_samples), bench_nalloc); | ||
| bench_accum_secs = 0; |
There was a problem hiding this comment.
xcalloc() arguments appear swapped: the function takes (nmemb, size), but this call passes (sizeof(*bench_samples), bench_nalloc). This will allocate an unexpectedly large buffer (nmemb=sizeof(timespec), size=8192) and likely OOM/crash; swap the arguments so you allocate bench_nalloc samples.
Comment on lines
+761
to
+767
| /* median */ | ||
| qsort(bench_samples, bench_nruns, sizeof(*bench_samples), tscmp); | ||
| i = bench_nruns / 2; | ||
| med_spr = tstod(&bench_samples[i]); | ||
| if (bench_nruns > 1 && bench_nruns & 1) | ||
| med_spr = (med_spr + tstod(&bench_samples[i - 1])) / 2.0; | ||
| med_rps = (med_spr == 0.0) ? INFINITY : 1.0/med_spr; |
There was a problem hiding this comment.
Median calculation looks incorrect: it averages the two middle samples when bench_nruns is odd (bench_nruns & 1), but the average-of-two should be used when the run count is even. As written, odd-sized samples will report an inflated median and even-sized samples won’t be averaged.
Comment on lines
+771
to
+778
| /* std. dev */ | ||
| std_dev = 0; | ||
| for (i = 0; i < bench_nruns; i++) { | ||
| std_dev = tstod(&bench_samples[i]) - mean_spr; | ||
| std_dev *= std_dev; | ||
| } | ||
| std_dev /= (double)bench_nruns; | ||
| std_dev = sqrt(std_dev); |
There was a problem hiding this comment.
Std. dev computation doesn’t accumulate across samples: the loop overwrites std_dev each iteration instead of summing squared deltas, so the result only reflects the last sample. Accumulate (e.g., add squared deltas) before dividing by bench_nruns.
Comment on lines
+709
to
+717
| void | ||
| bench_case_start(const char *file, int line) | ||
| { | ||
| clock_gettime(CLOCK_REALTIME, &bench_start_time); | ||
| } | ||
|
|
||
| void | ||
| bench_case_finish(const char *file, int line) | ||
| { |
There was a problem hiding this comment.
bench_case_start() and bench_case_finish() accept file/line parameters but don’t use them. If the build enables -Wunused-parameter, this will introduce warnings in the test helper; consider marking them unused (cast to void or apply an unused attribute).
Comment on lines
+304
to
+307
| test "x${UNITTEST_VERBOSE}" = "x" || ARGS="$$ARGS -v"; \ | ||
| test "x${UNITTEST_BENCH_DETAIL}" = "x" || ARGS="$$ARGS -B"; \ | ||
| test "x${UNITTEST_BENCH_ONLY}" = "x" || ARGS="$$ARGS -O ${UNITTEST_BENCH_ONLY}"; \ | ||
| $$V ${.OBJDIR}/unittests/sshbuf/test_sshbuf $${ARGS}; \ |
There was a problem hiding this comment.
UNITTEST_BENCH_ONLY is appended to ARGS without quoting. If the pattern contains spaces or glob characters, the shell will split/expand it and -O will receive the wrong value. Quote the value when constructing ARGS (similar to Makefile.inc).
| ;; | ||
| setup) | ||
| if /cygdrive/c/setup.exe -q -P `echo "$PACKAGES" | tr ' ' ,`; then | ||
| if /cygdrive/d/setup.exe -q -P `echo "$PACKAGES" | tr ' ' ,`; then |
There was a problem hiding this comment.
Cygwin setup is now hardcoded to /cygdrive/d/setup.exe. If the runner image (or local dev env) only has setup.exe on C: (the previous path), package installation will fail. Consider probing for both locations (or using an env var) and erroring with a clear message if neither exists.
Suggested change
| if /cygdrive/d/setup.exe -q -P `echo "$PACKAGES" | tr ' ' ,`; then | |
| cygwin_setup_exe="${CYGWIN_SETUP_EXE:-}" | |
| if [ -z "$cygwin_setup_exe" ]; then | |
| if [ -x /cygdrive/d/setup.exe ]; then | |
| cygwin_setup_exe=/cygdrive/d/setup.exe | |
| elif [ -x /cygdrive/c/setup.exe ]; then | |
| cygwin_setup_exe=/cygdrive/c/setup.exe | |
| else | |
| echo "Could not find Cygwin setup.exe. Set CYGWIN_SETUP_EXE or install setup.exe at /cygdrive/d/setup.exe or /cygdrive/c/setup.exe." | |
| exit 1 | |
| fi | |
| elif [ ! -x "$cygwin_setup_exe" ]; then | |
| echo "Configured CYGWIN_SETUP_EXE is not executable: $cygwin_setup_exe" | |
| exit 1 | |
| fi | |
| if "$cygwin_setup_exe" -q -P "$(echo "$PACKAGES" | tr ' ' ,)"; then |
Comment on lines
+489
to
+491
| if ((r = fwrite(sshbuf_ptr(entry), sshbuf_len(entry), 1, f)) != 1) { | ||
| error_f("fwrite: %s", strerror(errno)); | ||
| goto out; |
There was a problem hiding this comment.
fwrite() returns size_t, but the result is stored in an int. While you’re writing a single item here, using size_t for the return value avoids signed/width warnings and matches the standard library API.
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Tess Gauthier (tgauth)
force-pushed
the
scratch-merge-v10.3P1-20260420
branch
from
June 26, 2026 16:17
5e452f3 to
b03da9a
Compare
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
Batch 27 (V_10_4_P1 merge): upstream added libcrux-mlkem-mldsa.c and ssh-mldsa-eddsa.c to LIBSSH_OBJS for ML-DSA44/Ed25519 composite signature support. Register both in the Windows libssh project alongside kexmlkem768x25519.c.
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
config.h.vs defined USE_SNTRUP761X25519 and USE_MLKEM768X25519 but not USE_MLDSA, which defines.h enables together only for compilers advertising VLAs/decl-after-code (not MSVC). Without it, sshkey.c never registered sshkey_mldsa44_ed25519_impl, so sshkey_new(KEY_MLDSA44_ED25519) returned NULL and unittest-sshkey failed at test #6. The ML-DSA sources (ssh-mldsa-eddsa.c, libcrux-mlkem-mldsa.c) were already in libssh.vcxproj, so enabling the define links cleanly. unittest-sshkey now passes 98 tests on ARM64 Release.
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
…merge-v10.3P1-20260420
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
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.
PR Summary
PR Context