Skip to content

fix(tests): avoid unsigned underflow in DH group loop - #11050

Open
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/test-dh-group-loop-underflow
Open

fix(tests): avoid unsigned underflow in DH group loop#11050
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/test-dh-group-loop-underflow

Conversation

@MarkAtwood

@MarkAtwood MarkAtwood commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Fixes jenkins issue #558https://jenkins-supervisor.wolfssl.com/#/open-issues/558
(root-cause task #556: https://jenkins-supervisor.wolfssl.com/#/open-issues/556)


Problem

test_wc_DhGenerateKeyPair_and_Agree builds a groups[] array from the enabled HAVE_FFDHE_* macros, with a trailing 0 sentinel so the array is never empty, and then subtracts that sentinel back off in the loop bound:

static const int groups[] = {
#ifdef HAVE_FFDHE_2048
    WC_FFDHE_2048,
#endif
    ...
    0 /* keep the array non-empty when no HAVE_FFDHE_* is enabled */
};
size_t i;
for (i = 0; i < sizeof(groups) / sizeof(groups[0]) - 1; i++) {

When no HAVE_FFDHE_* is defined the count is 1, so the bound constant-folds to size_t i < 0 — an unsigned value compared against zero. GCC flags it under -Wtype-limits, and since configure enables -Werror for any VCS checkout (m4/ax_harden_compiler_flags.m4), the build fails outright:

tests/api/test_dh.c:569:19: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
  569 |     for (i = 0; i < sizeof(groups) / sizeof(groups[0]) - 1; i++) {
      |                   ^

This is not hypothetical — it has been breaking a nightly small-footprint build for 11 consecutive runs. The configuration that trips it enables dh and rsa with all curves disabled, so HAVE_FFDHE_2048 is never defined (configure.ac gates it behind TLS 1.3 or supported-curves).

Fix

Compare i + 1 against the element count rather than i against count - 1. The subtraction can no longer underflow.

Verification

Arithmetically identical for every array size. Iteration counts measured before and after on gcc 9.4.0:

groups[] before after
{0} 0 0
{g,0} 1 1
{g,g,0} 2 2
{g,g,g,0} 3 3

The pre-patch form fails to compile at size 1 under -Werror -Wtype-limits; the patched form compiles clean and behaves identically everywhere else.

Worth noting separately

The loop body is already dead in the no-FFDHE configuration — that build runs no DH group exchange at all, and passes. This PR only fixes the build break; whether such a configuration should skip the test loudly is a separate coverage question I'm happy to follow up on.

groups[] carries a trailing 0 sentinel so the array is never empty when
no HAVE_FFDHE_* is defined, and the loop bound subtracts it back off.
In that configuration the count is 1, so the bound folds to

    size_t i < 1 - 1

i.e. an unsigned value compared against 0. GCC diagnoses this as
-Wtype-limits, and builds using -Werror (which configure enables for any
VCS checkout) fail:

    tests/api/test_dh.c:569:19: error: comparison of unsigned expression
    < 0 is always false [-Werror=type-limits]

Compare i + 1 against the element count instead. This never underflows
and is arithmetically identical for every array size; verified the
iteration count is unchanged for 0, 1, 2 and 3 configured groups.

Note the loop body is already dead in that configuration, so a build
with no FFDHE group enabled runs no DH group exchange at all.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a compile-time warning/error in the DH API test when no HAVE_FFDHE_* groups are enabled, by rewriting the loop bound to avoid an unsigned underflow that can trigger -Wtype-limits under -Werror.

Changes:

  • Update the groups[] iteration condition to use i + 1 < element_count instead of i < element_count - 1, preventing underflow when the array contains only the sentinel.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MarkAtwood

Copy link
Copy Markdown
Contributor Author

@danielinux — assigning you since this touches the loop from 2e1d38d ("tests: MC/DC decision coverage for dh.c and dsa.c"). The fix itself is mechanical, but there's a coverage question that's yours to call.

The build break is just the unsigned underflow: with no HAVE_FFDHE_* defined, groups[] is the lone sentinel, sizeof(groups)/sizeof(groups[0]) - 1 folds to 0, and size_t i < 0 trips -Wtype-limits under -Werror. Comparing i + 1 against the count fixes that, and I verified the iteration count is unchanged for 0/1/2/3 configured groups.

The part worth your judgement: in that same configuration the loop body never executes, so test_wc_DhGenerateKeyPair_and_Agree passes while performing no DH group exchange at all. Compiling clean and testing nothing look identical from the outside, and for an MC/DC coverage test that seems like the opposite of the intent.

Options, in your call:

  • leave as-is — the config genuinely has no FFDHE groups to exercise, and silence is acceptable
  • skip loudly, so the report distinguishes "no groups configured" from "groups tested"
  • guard the whole block on defined(HAVE_FFDHE_2048) || defined(HAVE_FFDHE_3072) || defined(HAVE_FFDHE_4096) and drop the sentinel, making the dependency explicit

I kept this PR to the one-line build fix rather than pre-empting that decision. Happy to fold in whichever you prefer.

For context on urgency: this has been failing nightly-memusage-v2 for 11 consecutive builds — its small-footprint low_rsa2048 config disables all curves, so HAVE_FFDHE_2048 never gets defined. The job's own --enable-all build is unaffected, which is why it wasn't obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants