Repository navigation
Build out-of-tree CI configs with -Werror and the full warning set - #11859
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Newly enforcing fatal warnings across roughly 250 configurations warrants full multi-platform CI validation and maintainer review.
No open issues 🎉
What changed in this PR
This PR makes out-of-tree CI builds retain configure’s warning flags and enforce -Werror for source checkouts, while addressing warnings exposed by the stricter builds.
Changes:
- Detect version-control metadata in
$srcdirso out-of-tree checkouts receive-Werror. - Preserve configure’s CFLAGS through additive CI flags and remove obsolete overrides.
- Fix feature guards, signed-char comparisons, function-pointer loading, and declaration ordering.
| File | Description |
|---|---|
wolfcrypt/test/test.c |
Moves ECC test-vector declarations before executable statements. |
wolfcrypt/src/wc_pkcs11.c |
Uses a union for dynamically loaded function pointers. |
wolfcrypt/src/random.c |
Excludes unused local health-test code with custom block generation. |
tests/api/test_dtls.c |
Aligns the cookie test’s feature guard and fixes char comparisons. |
m4/ax_vcs_checkout.m4 |
Detects source checkouts through $srcdir. |
.github/workflows/tinytls13.yml |
Makes configuration-specific compiler flags additive. |
.github/workflows/os-check.yml |
Preserves configure’s flags when adding strict warnings. |
.github/workflows/enterprise.yml |
Switches strict warning flags to --extra-cflags. |
.github/workflows/afalg.yml |
Uses additive warning flags for AF_ALG builds. |
.github/scripts/parallel-make-check.py |
Warns about ineffective empty cflags entries. |
.github/configs/os-check-macos.json |
Removes obsolete empty overrides and updates explanatory text. |
.github/configs/os-check-linux.json |
Removes obsolete flag overrides and simplifies related comments. |
Review effort: Balanced
|
Jenkins retest this please - flaky test |
|
@wolfSSL-Fenrir-bot review balanced |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The C changes are small, but they touch library code in the TLS key exchange, the RNG, PKCS#11 loading and XMSS, and the m4 change turns on -Werror for every out-of-tree build of a checkout, so a maintainer should still take a look.
What was reviewed:
- random.c: every caller of
wc_RNG_HealthTestLocal(Hash_DRBG init, reseed, and theWC_RNG_HAVE_NEXT_SEEDpath) is already excluded whenCUSTOM_RAND_GENERATE_BLOCKis defined, so the new guard can't hide a function that is still in use. - wc_pkcs11.c: the
dlsymunion keeps the same behavior. pkcs12.c: checkingsdBufinstead ofsignDataonly matters when encoding was skipped. In wc_xmss_impl.c, the(word32)kcasts happen only whilek >= 0. test.c: the KAT calls still run in the same order under the same AES/X963 guards. - m4: the
$srcdir/.gitprobe is intentional. Packagers who build a git checkout out-of-tree will now get -Werror, but that is the purpose of the PR.
Extended reasoning...
The PR changes 15 files. Most are CI workflow and config changes. The rest are small warning fixes in src/internal.c (DoClientKeyExchange), wolfcrypt random.c, pkcs12.c, wc_pkcs11.c and wc_xmss_impl.c, plus test files. None of the fixes changes runtime behavior in an existing configuration. Still, they sit in TLS and crypto code paths, and the m4 change makes the build stricter for out-of-tree builds of a checkout. That combination of crypto-adjacent code and a project-wide build-policy change is why this deferred rather than approved.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11859
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): .github/configs/os-check-linux.json, .github/configs/os-check-macos.json, .github/scripts/parallel-make-check.py, .github/workflows/afalg.yml, .github/workflows/enterprise.yml, .github/workflows/os-check.yml, .github/workflows/tinytls13.yml, m4/ax_vcs_checkout.m4
Coverage: 7 of 7 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Balanced
|
This has some overlap with the just merged #11887, which also updated I think The |
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
Please review merge conflicts carefully, I think some (but not all) of these fixes are in master now, even though not all are presenting merge conflicts.
os-check handed its flags to parallel-make-check.py with --cflags, which becomes a make-time CFLAGS= and replaces the CFLAGS configure chose. That dropped wolfSSL's own warning set, so every os-check build compiled with neither -Wall nor -Wextra. Switching to --extra-cflags keeps that set, -Werror included now that configure adds it to out-of-tree builds. The entries that carried their own cflags or ldflags only repeated the job flags to survive the replacement: the two -m32 entries already pass CFLAGS=-m32 to configure, which the link line picks up as well, and cryptocb-curve25519-no-key-export only added -Werror. The user_settings entries and apple-native-cert-validation set an empty "cflags" to stay off the job flags. That key no longer opts out of anything, so it is dropped, and those entries now build with the job's -pedantic set and -Werror too.
enterprise.yml handed its flags to parallel-make-check.py with --cflags, the same way os-check did, so the enterprise configs compiled without wolfSSL's warning set and without -Werror. Pass them with --extra-cflags as os-check now does.
Both afalg configs and three tinytls13 configs set a per-entry "cflags", which replaces configure's CFLAGS at make time and takes -Wall, -Wextra and -Werror with it. Use "extra_cflags" so the flags are added instead.
An empty "cflags" only overrides a global --cflags. Without one it does nothing, yet it reads like an opt-out from the job flags, which is how the 25 dead entries removed from the os-check configs came about. Warn about it and name "extra_cflags": "" as the opt-out from --extra-cflags.
test_dtls_cookie_flip_in_cb required TLS 1.2 while its callback, test_dtls_ch_flip_cb, is compiled whenever TLS 1.2 or DTLS 1.3 is available. A --disable-tlsv12 --enable-dtls13 build defined the callback without using it and failed with -Wunused-function. Give the test the same guard as its neighbours. It passes on DTLS 1.3 unchanged, so it gains coverage instead of hiding the callback.
wc_RNG_HealthTestLocal() and the SHA-512 vectors only it reads were built under HAVE_HASHDRBG, while every caller also requires !CUSTOM_RAND_GENERATE_BLOCK. Defining both, as the rng-custom-block-hashdrbg configuration does, left the function unused and failed the build with -Wunused-function.
wc_Pkcs11_Initialize_v3() cast the void* from dlsym() straight to CK_C_GetInterface and CK_C_GetFunctionList. ISO C does not allow that conversion, so -pedantic -Werror builds of the dynamic loader failed. Go through a union of the data pointer and both function pointer types.
The AES-128 vector was declared after the AES-256 case had already run, a declaration after a statement that -Wdeclaration-after-statement rejects whenever both key sizes are enabled. Declare all vectors first and run the cases after them, which also retires the bare block the X9.63 vector was declared in.
The dummy get_peer callbacks store -1 in ip[0] and the set_peer callbacks compared ip[0] against -1. Where char is unsigned, as on Linux aarch64, the stored byte reads back as 255, so the comparison was always true and -Wtype-limits failed the build. Compare against (char)-1.
62a0437 to
074664f
Compare
|
@philljj should all be fixed now. |
|
Retest this please. (fips windows test hung or something) |
|
tests good with multi-test with |
os-check, enterprise, afalg and tinytls13 pass
cflagstoparallel-make-check.py, which runsmake CFLAGS=...and so replaces configure's warning set, including-Werror. For example, theno-tlsv12-dtls13os-check leg passes on master, although that build fails on an unusedtest_dtls_ch_flip_cb.19e71c8 already makes configure look for
.gitin$srcdir, so out-of-tree builds of a checkout get-Werror. This PR keeps those flags in the builds that overrode them:--extra-cflagsinstead of--cflags, and afalg and tinytls13 use"extra_cflags"instead of"cflags"."cflags": ""opt-outs and two"ldflags": "-m32"keys from the os-check configs.parallel-make-check.pywarns about an empty"cflags".Fixes for what the full warning set exposed:
test_dtls.c:test_dtls_cookie_flip_in_cbuses its callback's guard, so DTLS 1.3 builds without TLS 1.2 build again (from DTLS 1.3: accept a fragmented first ClientHello when cookies are disabled #11475). The export tests compareip[0]with(char)-1, sincecharis unsigned on Linux aarch64.random.c:wc_RNG_HealthTestLocalis not compiled withCUSTOM_RAND_GENERATE_BLOCK, where it has no caller.wc_pkcs11.c:dlsymresults go through a union instead of an object-to-function-pointer cast (-pedantic).test.c:ecc_encrypt_gcm_katdeclares its vectors before code.