From 156b7db33c5ecee68bed112db3399dc6c27e1fbf Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 9 Sep 2026 08:31:53 +0200 Subject: [PATCH] tests: check API availability guards before the build does tests/api is one binary compiled in every CI configuration, so a test calling an API the build did not compile is not a test failure -- it is a link error that takes the whole binary down. It is also invisible to header inspection, because wolfSSL declares plenty of API unconditionally and implements it under a narrower condition. That combination broke CI four separate times on this branch, each time found by CI rather than locally, and each time the fix was the same: name what the build provides, not what the test needs. check-api-guards.py walks the enclosing #if chain of every call site and requires the macros the IMPLEMENTATION carries. It is a whitelist rather than a parse of ssl.h on purpose: the mapping from symbol to implementation guard cannot be derived from the declaration, which is the whole problem. Two things make it usable rather than noisy: It only looks at call sites this branch changed. Run over everything it reports 28 long-standing sites that are fine in practice because the configurations that would break them are not built; auditing those is a different job, and --all still does it. It knows which macros imply TLS. A block under WOLFSSL_TLS13 or HAVE_SNI cannot also need !defined(NO_TLS) spelled out, and comments and string literals are blanked before matching, since these files discuss the very API names being searched for. It refuses to run against a ref it cannot resolve rather than reporting success, because a shallow checkout would otherwise make every diff empty and the check would pass without looking at anything. The workflow checks out with fetch-depth: 0 for that reason, and runs the check before the smoke build -- it needs no build and costs a second. Verified both directions: clean on this branch, and it reports the exact site when !defined(NO_TLS) is removed from a guard that needs it. --- .github/workflows/whitebox-smoke.yml | 15 ++ tests/api/check-api-guards.py | 212 +++++++++++++++++++++++++++ tests/api/include.am | 3 +- 3 files changed, 229 insertions(+), 1 deletion(-) create mode 100755 tests/api/check-api-guards.py diff --git a/.github/workflows/whitebox-smoke.yml b/.github/workflows/whitebox-smoke.yml index 8465dab657..138f43a578 100644 --- a/.github/workflows/whitebox-smoke.yml +++ b/.github/workflows/whitebox-smoke.yml @@ -36,6 +36,10 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@v4 + with: + # check-api-guards.py diffs against the base branch, so it + # needs more than the default shallow checkout. + fetch-depth: 0 - name: Install build dependencies run: | @@ -51,6 +55,17 @@ jobs: CPPFLAGS=-DWOLFSSL_TEST_STATIC_BUILD make -j"$(nproc)" + - name: Check API availability guards + # tests/api is one binary built in every CI configuration, so a test + # that calls an API the build did not compile is a link error that + # takes the whole binary down -- and it is invisible to anything that + # only reads headers, because plenty of API is declared unconditionally + # and implemented under a narrower condition. This checks the call + # sites this branch changed against the conditions the implementations + # actually carry. It costs a second and needs no build, so it runs + # before the smoke build rather than after it. + run: python3 tests/api/check-api-guards.py origin/${{ github.base_ref || 'master' }} + - name: Run white-box smoke # smoke-expected.txt is generated with gcc; six TUs build under clang # and not gcc, so the compiler has to match or the run reports false diff --git a/tests/api/check-api-guards.py b/tests/api/check-api-guards.py new file mode 100755 index 0000000000..d43317c301 --- /dev/null +++ b/tests/api/check-api-guards.py @@ -0,0 +1,212 @@ +#!/usr/bin/env python3 +# +# check-api-guards.py [--all] [base-ref] +# +# Every entry in tests/api is compiled into the one unit.test binary, in every +# configuration CI builds. A test that calls an API the build did not compile +# is not a test failure -- it is a link error that takes the whole binary down, +# and it is invisible to anything that only reads headers, because wolfSSL +# declares plenty of API unconditionally and implements it under a narrower +# condition. That combination has broken CI here four separate times: a guard +# that names what the test NEEDS rather than what the build PROVIDES. +# +# This checks the other direction. For each API below it walks every call site's +# enclosing #if chain and requires the macros the IMPLEMENTATION requires. Run +# it over the test sources; it exits non-zero if a call site is not covered. +# +# Adding an entry: find where the function is defined (not declared) and copy +# the conditions around it. "requires_off" are macros that must be excluded, +# "requires_on" macros that must be present. +# +# It is deliberately a whitelist rather than a parse of ssl.h: the mapping from +# symbol to implementation guard cannot be derived from the declaration, which +# is the entire problem it exists to catch. +# +# By default it only looks at call sites on lines this branch added or changed +# against the base ref (origin/master), which is what makes it usable as a +# pre-push check. Running it over everything reports plenty of long-standing +# call sites that are fine in practice, because the configurations that would +# break them are not built -- auditing those is a different job. --all does +# that anyway. +import re +import sys +import glob + +# symbol -> (must be excluded, must be defined), from the definition site +API = { + # src/ssl.c, under !NO_WOLFSSL_CLIENT && !NO_TLS + 'wolfSSLv23_client_method': (['NO_TLS', 'NO_WOLFSSL_CLIENT'], []), + 'wolfSSLv23_server_method': (['NO_TLS', 'NO_WOLFSSL_SERVER'], []), + # src/tls.c, additionally under WOLFSSL_DTLS && !WOLFSSL_NO_TLS12: + # DTLS 1.2 is built out of the TLS 1.2 code + 'wolfDTLSv1_2_client_method': (['NO_WOLFSSL_CLIENT', 'WOLFSSL_NO_TLS12'], + ['WOLFSSL_DTLS']), + 'wolfDTLSv1_2_server_method': (['NO_WOLFSSL_SERVER', 'WOLFSSL_NO_TLS12'], + ['WOLFSSL_DTLS']), + # src/ssl_api_ext.c: each sits under !NO_TLS as well as its own feature + 'wolfSSL_UseSNI': (['NO_TLS'], ['HAVE_SNI']), + 'wolfSSL_CTX_UseSNI': (['NO_TLS'], ['HAVE_SNI']), + 'wolfSSL_SNI_GetRequest': (['NO_TLS', 'NO_WOLFSSL_SERVER'], ['HAVE_SNI']), + 'wolfSSL_SNI_GetFromBuffer': (['NO_TLS', 'NO_WOLFSSL_SERVER'], ['HAVE_SNI']), + 'wolfSSL_UseSupportedCurve': (['NO_TLS'], ['HAVE_SUPPORTED_CURVES']), + 'wolfSSL_CTX_UseSupportedCurve': (['NO_TLS'], ['HAVE_SUPPORTED_CURVES']), + # wolfcrypt/src/memory.c, under USE_WOLFSSL_MEMORY -- which --enable-leantls + # removes by way of WOLFSSL_LEANPSK + 'wolfSSL_SetAllocators': ([], ['USE_WOLFSSL_MEMORY']), + 'wolfSSL_GetAllocators': ([], ['USE_WOLFSSL_MEMORY']), +} + + +def strip_comments(text): + """Blank out comments and string literals, keeping every newline so line + numbers still line up. Without this the scan matches the API names in the + explanatory comments these tests are full of.""" + out = [] + i, n = 0, len(text) + while i < n: + c = text[i] + if c == '/' and i + 1 < n and text[i + 1] == '*': + j = text.find('*/', i + 2) + j = n if j < 0 else j + 2 + out.append(''.join(ch if ch == '\n' else ' ' for ch in text[i:j])) + i = j + elif c == '/' and i + 1 < n and text[i + 1] == '/': + j = text.find('\n', i) + j = n if j < 0 else j + out.append(' ' * (j - i)) + i = j + elif c in '"\'': + q, j = c, i + 1 + while j < n and text[j] != q: + j += 2 if text[j] == '\\' else 1 + j = min(j + 1, n) + out.append(''.join(ch if ch == '\n' else ' ' for ch in text[i:j])) + i = j + else: + out.append(c) + i += 1 + return ''.join(out) + + +def guard_chain(lines, upto): + """The #if directives open at line `upto`, joined, continuations included.""" + stack = [] + for i, line in enumerate(lines[:upto], 1): + s = line.strip() + if re.match(r'#\s*if', s): + text, j = [], i - 1 + while True: + text.append(lines[j].strip()) + if not lines[j].rstrip().endswith('\\'): + break + j += 1 + stack.append(' '.join(text)) + elif re.match(r'#\s*endif', s): + if stack: + stack.pop() + elif re.match(r'#\s*el(se|if)', s): + if stack: + stack[-1] = s + return ' '.join(stack) + + +# Macros that are only ever defined in a build that has TLS, so a block already +# guarded by one of them cannot also need !defined(NO_TLS) spelled out. +IMPLIES_TLS = ( + 'WOLFSSL_TLS13', 'WOLFSSL_DTLS', 'WOLFSSL_DTLS13', 'HAVE_SNI', 'HAVE_ALPN', + 'HAVE_SESSION_TICKET', 'HAVE_SECURE_RENEGOTIATION', 'HAVE_MAX_FRAGMENT', + 'HAVE_SUPPORTED_CURVES', 'HAVE_EXTENDED_MASTER', 'HAVE_TRUSTED_CA', + 'HAVE_ENCRYPT_THEN_MAC', 'HAVE_SERVER_RENEGOTIATION_INFO', + 'HAVE_CERTIFICATE_STATUS_REQUEST', 'HAVE_TLS_EXTENSIONS', 'HAVE_ECH', +) + + +def require_ref(base): + """Refuse to run against a ref git cannot resolve. + + A shallow checkout has no base branch, and then every diff comes back + empty and the check passes without looking at anything -- which is worse + than not running it, because it reports success. Fail loudly instead. + """ + import subprocess + r = subprocess.run(['git', 'rev-parse', '--verify', '--quiet', base + '^{commit}'], + capture_output=True, text=True) + if r.returncode != 0: + sys.stderr.write( + f"check-api-guards: cannot resolve '{base}'.\n" + f" The diff scope needs it. In CI, check out with fetch-depth: 0;\n" + f" locally, fetch the base branch, or pass --all to scan every\n" + f" call site instead.\n") + sys.exit(2) + + +def changed_lines(path, base): + """Line numbers this branch added or changed in path.""" + import subprocess + out = subprocess.run(['git', 'diff', '-U0', f'{base}...HEAD', '--', path], + capture_output=True, text=True).stdout + hit = set() + for m in re.finditer(r'^@@ -\S+ \+(\d+)(?:,(\d+))? @@', out, re.M): + start = int(m.group(1)) + count = int(m.group(2)) if m.group(2) else 1 + hit.update(range(start, start + count)) + return hit + + +def check(path, only=None): + lines = strip_comments(open(path, errors='replace').read()).split('\n') + bad = [] + for i, line in enumerate(lines, 1): + if only is not None and i not in only: + continue + for sym, (off, on) in API.items(): + if not re.search(r'\b' + re.escape(sym) + r'\s*\(', line): + continue + chain = guard_chain(lines, i) + miss_off = [m for m in off + if f'!defined({m})' not in chain and f'ifndef {m}' not in chain] + miss_on = [m for m in on + if f'defined({m})' not in chain and f'ifdef {m}' not in chain] + # WOLFSSL_DTLS implies TLS is compiled in, so a DTLS-guarded block + # never needs !NO_TLS spelled out as well. + if any(f'defined({m})' in chain or f'ifdef {m}' in chain + for m in IMPLIES_TLS): + miss_off = [m for m in miss_off if m != 'NO_TLS'] + if miss_off or miss_on: + bad.append((i, sym, miss_off, miss_on)) + return bad + + +def main(): + args = [a for a in sys.argv[1:]] + scan_all = '--all' in args + if scan_all: + args.remove('--all') + base = args[0] if args else 'origin/master' + if not scan_all: + require_ref(base) + paths = sorted(glob.glob('tests/api/test_*.c')) + total = 0 + for path in paths: + only = None if scan_all else changed_lines(path, base) + if only is not None and not only: + continue + for line, sym, off, on in check(path, only): + need = [] + if off: + need.append('!defined(' + '), !defined('.join(off) + ')') + if on: + need.append('defined(' + '), defined('.join(on) + ')') + print(f'{path}:{line}: {sym} needs {" and ".join(need)}') + total += 1 + scope = 'every call site' if scan_all else f'call sites changed since {base}' + if total: + print(f'\n{total} call site(s) reachable in a build that does not ' + f'implement the API ({scope})') + return 1 + print(f'api guards: {scope} covered') + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/tests/api/include.am b/tests/api/include.am index efb49b40c3..4e60f7a430 100644 --- a/tests/api/include.am +++ b/tests/api/include.am @@ -147,7 +147,8 @@ tests_unit_test_SOURCES += tests/api/test_tls13_bounds.c tests_unit_test_SOURCES += tests/api/test_tls13_features.c endif -EXTRA_DIST += tests/api/api.h +EXTRA_DIST += tests/api/check-api-guards.py \ + tests/api/api.h EXTRA_DIST += tests/api/api_decl.h EXTRA_DIST += tests/api/test_md2.h EXTRA_DIST += tests/api/test_md4.h