cmake: add more build options in line with automake#10834
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10834
No scan targets match the changed files in this PR. Review skipped.
|
Jenkins: retest this please |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 4 total — 3 posted, 1 skipped
Posted findings
- [Medium] App-bundle sub-option forcing runs before options are defined, degrading cache entries —
CMakeLists.txt:250-534 - [Medium] Inconsistent WOLFSSL_LINUX_KM gating silently no-ops some kernel options —
CMakeLists.txt:2940-2960 - [Low] Repeated empty placeholder comment blocks in options.h.in —
cmake/options.h.in:811-817
Skipped findings
- [Medium] New build options ship with no test/CI coverage
Review generated by Skoll via Claude/Codex
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Skoll review (follow-up). Thanks for the earlier fixes — I confirmed all three of my previous comments are addressed (force_option via GLOBAL property preserves cache type/help; LINUXKM_PIE/BENCHMARKS now warn when WOLFSSL_LINUX_KM is off; the empty options.h.in parity headers are gone).
One blocking issue remains, plus a few suggestions/nits below.
SparkiDev
force-pushed
the
cmake_update_1
branch
2 times, most recently
from
0394385 to
da5c186
Compare
|
Jenkins: retest this please |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 1 posted, 2 skipped
Posted findings
- [High] New RC2/ASCON/SM2/SM3/SM4 options define feature macros but never compile their source files —
CMakeLists.txt:2967
Skipped findings
- [Medium] force_option silently overrides an explicit user setting with no diagnostic
- [Low] A force_option for an option never declared via add_option silently has no effect
Review generated by Skoll via Claude/Codex
|
Jenkins: retest this please |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] WC_32BIT_CPU emitted on compile line but missing #cmakedefine in options.h.in —
CMakeLists.txt:555-557 (and missing entry in cmake/options.h.in near line 242) - [Low] [review-security] SM2/SM3/SM4 sources selected under FIPS v2, deviating from automake !BUILD_FIPS_V2_PLUS guard —
cmake/functions.cmake:924-964 - [Low] [review] Non-numeric WOLFSSL_MAX_RSA_BITS / WOLFSSL_MAX_ECC_BITS bypass range validation —
CMakeLists.txt:3705-3730 (Maximum key size options block) - [Info] [review] set_wolfssl_definitions PARENT_SCOPE change adds compile-line defines for PQC builds (confirm intent) —
cmake/functions.cmake:1325-1329 - [Info] [review-security] force_option applied only via add_option; forced non-add_option options would be silently ignored —
cmake/functions.cmake:18-41
Review generated by Skoll
| list(APPEND LIB_SOURCES wolfcrypt/src/ascon.c) | ||
| endif() | ||
|
|
||
| # ShangMi SM2/SM3/SM4. The implementation files are provided by the |
There was a problem hiding this comment.
🔵 [Low] SM2/SM3/SM4 sources selected under FIPS v2, deviating from automake !BUILD_FIPS_V2_PLUS guard · Logic
The new ShangMi source-selection block guards sm2.c/sm3.c/sm4.c with only if(NOT BUILD_FIPS_V5). The accompanying comment claims 'this block is already inside NOT BUILD_FIPS_V2', but it is not: the enclosing scope is if(NOT BUILD_FIPS_RAND) opened at line 839, with no NOT BUILD_FIPS_V2 wrapper between line 839 and the SM block at 929. The authoritative automake rule in src/include.am guards all three sources with !BUILD_FIPS_V2_PLUS (excludes FIPS v2 AND v5). For a FIPS v2 build that also enables SM2/SM3/SM4, the cmake build would compile sm2.c/sm3.c/sm4.c while automake excludes them: with external wolfsm sources present this places non-FIPS crypto inside the FIPS module boundary (certification-relevant); without wolfsm the in-tree stubs #error and break the build. Also sp_sm2_arm32.c/sp_sm2_arm64.c lack the extra !BUILD_FIPS_V2 inner guards automake applies, and sp_sm2_x86_64_asm.S is gated on WOLFSSL_X86_64_BUILD_ASM whereas automake adds it unconditionally under BUILD_SP_X86_64.
Fix: Wrap the SM block in if(NOT BUILD_FIPS_V2 AND NOT BUILD_FIPS_V5) (i.e. reproduce !BUILD_FIPS_V2_PLUS) to match automake, and fix the misleading comment. Consider mirroring automake's inner !BUILD_FIPS_V2 guards on the sp_sm2_arm32/arm64 entries.
There was a problem hiding this comment.
Fixed.
Changed the guard to if(NOT BUILD_FIPS_V2 AND NOT BUILD_FIPS_V5
| "Maximum bit size to support for ECC algorithms (112-1024)") | ||
|
|
||
| set(WOLFSSL_MPI_MAX_KEY_BITS "") | ||
| if(WOLFSSL_MAX_RSA_BITS) |
There was a problem hiding this comment.
🔵 [Low] Non-numeric WOLFSSL_MAX_RSA_BITS / WOLFSSL_MAX_ECC_BITS bypass range validation · bug
These are raw cache STRING variables validated with if(WOLFSSL_MAX_RSA_BITS LESS 1024 OR WOLFSSL_MAX_RSA_BITS GREATER 16384). CMake's LESS/GREATER return false when either operand is not a valid integer, so a non-numeric value (e.g. -DWOLFSSL_MAX_RSA_BITS=foo) passes the range check silently and then flows straight into -DRSA_MAX_SIZE=foo on the compile line and #define RSA_MAX_SIZE foo in options.h, producing a confusing compile error deep in the crypto build rather than a clear configure-time diagnostic. The same applies to WOLFSSL_MAX_ECC_BITS (112-1024).
Fix: Add an integer-format check (e.g. MATCHES "^[0-9]+$") before the range comparison for both WOLFSSL_MAX_RSA_BITS and WOLFSSL_MAX_ECC_BITS so a malformed value fails cleanly at configure time.
There was a problem hiding this comment.
Fixed
Added NOT X MATCHES "^[0-9]+$" before the range check
| @@ -1240,6 +1323,10 @@ function(set_wolfssl_definitions SEARCH_VALUE RESULT) | |||
|
|
|||
| message(STATUS "Enabling ${SEARCH_VALUE}") | |||
| list(APPEND WOLFSSL_DEFINITIONS "-D${SEARCH_VALUE}") | |||
There was a problem hiding this comment.
⚪ [Info] set_wolfssl_definitions PARENT_SCOPE change adds compile-line defines for PQC builds (confirm intent) · question
The added set(WOLFSSL_DEFINITIONS "${WOLFSSL_DEFINITIONS}" PARENT_SCOPE) now propagates the appended -D\<SEARCH_VALUE> flag to the caller. Previously the append was confined to the function scope and lost, so macros forced through this helper (e.g. WOLFSSL_SHA3, WOLFSSL_SHAKE128/256, OPENSSL_EXTRA, HAVE_FALCON in the MLKEM/MLDSA/LMS/SLHDSA/FALCON blocks at CMakeLists.txt:936-1138) only became CMake variables (reaching options.h) but never reached the actual compile line. This is almost certainly the intended fix, but it is a behavior change: these PQC configurations now get additional -D flags on target_compile_definitions. Because the same macros may also arrive via options.h, verify there is no -Wmacro-redefined noise (-DWOLFSSL_SHA3 = 1 on the command line vs a bare #define WOLFSSL_SHA3 from options.h). Intra-list duplicates are collapsed by the later list(REMOVE_DUPLICATES), so intra-list duplication is not a concern.
Fix: Confirm the newly-added compile-line defines for MLKEM/MLDSA/LMS/SLHDSA/FALCON builds are intended and that no macro-redefinition warnings result when wolfssl/options.h is also included; if intended, no change needed.
| # that add_option() call runs. This keeps the option's declared type and help | ||
| # text intact (unlike creating the cache entry early), and makes the dependency | ||
| # explicit rather than relying on cross-file cache-precedence side effects. | ||
| function(force_option NAME VALUE) |
There was a problem hiding this comment.
⚪ [Info] force_option applied only via add_option; forced non-add_option options would be silently ignored · Logic
The force mechanism only takes effect when a later add_option(NAME ...) runs (it reads the WOLFSSL_FORCE_${NAME} global and calls override_cache). Any option a bundle forces that does NOT have a subsequent add_option() would have its force silently dropped. Every currently-forced option (WOLFSSL_AES/ARC4/CURVE25519/DES3/DSA/MD5/MD4/RIPEMD/CRL/OCSP/OCSPSTAPLING/OCSPSTAPLING_V2/OLD_TLS/DTLS/DTLS13/CRL_MONITOR/PKCALLBACKS/PKCS7/MD2/SRP) does have an add_option that runs after the bundle block, so this is currently correct. This is a latent maintenance hazard, not a present defect: a future bundle that force_option()'s an option set via plain set()/cache STRING (e.g. one of the raw-cache-STRING options) would be a no-op with no diagnostic.
Fix: Optionally add a post-configure sweep that warns if any WOLFSSL_FORCE_* global was never consumed by an add_option(), to catch future force_option targets that lack an add_option.
There was a problem hiding this comment.
Fixed.
Added the sweep as suggested.
| add_option("WOLFSSL_32BIT" | ||
| "Enables 32-bit support (default: disabled)" | ||
| "no" "yes;no") | ||
| if(WOLFSSL_32BIT) |
There was a problem hiding this comment.
🟠 [Medium] WC_32BIT_CPU emitted on compile line but missing #cmakedefine in options.h.in · Logic / convention
The PR adds if(WOLFSSL_32BIT) list(APPEND WOLFSSL_DEFINITIONS "-DWC_32BIT_CPU") endif() to CMakeLists.txt so WOLFSSL_32BIT now emits -DWC_32BIT_CPU. Unlike its sibling WOLFSSL_16BIT (whose macro WC_16BIT_CPU has both #undef/#cmakedefine in cmake/options.h.in lines 242-243), there is NO #cmakedefine WC_32BIT_CPU in cmake/options.h.in. Every entry of WOLFSSL_DEFINITIONS reaches the library compile line via target_compile_definitions(... PRIVATE) AND is normally reflected into the installed wolfssl/options.h via a matching #cmakedefine. Because this one is missing, the compiled library uses WC_32BIT_CPU while the generated wolfssl/options.h does not define it. A downstream consumer that configures with -DWOLFSSL_32BIT=yes and includes wolfssl/options.h (without itself defining WC_32BIT_CPU) sees a different CPU word-size/codepath configuration than the library it links against, risking type/ABI skew (e.g. sp_int digit sizing). This also violates the PR's own stated 3-place invariant (add_option + define in CMakeLists.txt, #cmakedefine in options.h.in). Severity views differ across modes: the review mode rated this Medium, the security mode rated it Low and noted that automake's --enable-32bit likewise only puts -DWC_32BIT_CPU into AM_CFLAGS (so it is arguably faithful to automake); the stricter Medium is retained here because it is inconsistent with the cmake design where all other added defines have options.h.in entries.
Fix: Add #undef WC_32BIT_CPU / #cmakedefine WC_32BIT_CPU to cmake/options.h.in (adjacent to the existing WC_16BIT_CPU entry) so the forced-32-bit setting is reflected in the generated wolfssl/options.h and matches the 16-bit handling and the PR's 3-place invariant; or intentionally document that WC_32BIT_CPU is compile-line-only to match automake AM_CFLAGS.
Note: Referenced line (
CMakeLists.txt:555-557 (and missing entry in cmake/options.h.in near line 242)) is outside the diff hunk. Comment anchored to nearest changed region.
There was a problem hiding this comment.
Fixed.
Added #undef/#cmakedefine WC_32BIT_CPU next to WC_16BIT_CPU.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Nice work. Skoll found a few useful things
#10834 (review)
Updates - ~100 individual options added (ciphers, TLS features, debug/test, caches, kernel-module, key-size numerics, asm) + fixed WOLFSSL_32BIT's missing define. - 37 app-integration bundles (openssh, nginx, haproxy, openvpn, wpas, apachehttpd, jni, wolfclu, wolfsentry, ...) - each force-enables its sub-options and emits its defines; all build clean and match ./configure --enable-X exactly (miss=0/extra=0). - Each option wired in 3 places: add_option + define in CMakeLists.txt, #cmakedefine in options.h.in, source selection where needed. Verified by real libwolfssl.so builds.
|
Jenkins: retest this please |
5 participants
Description
Updates
Testing
None