|WARNING| [v5] examples: add Wycheproof validation app

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169936

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-22

# DPDK Patch Review: Wycheproof Validation Example

## Correctness Issues

### Errors

1. **Resource leak on error path in `app_init()` (lines 208-214)**
   ```c
   if (s > session_size)
       session_size = s;
   ```
   If the loop over all devices finds a larger session size but then session pool creation fails, the code jumps to `error:` and attempts to free pools that were never created. The `env.session_pool` is set to NULL in the global initializer, so `rte_mempool_free(NULL)` is safe, but the logic is fragile.
   
   **This is actually correct** - `rte_mempool_free()` accepts NULL, and the pools are initialized to NULL in the global `env` structure. The error path correctly frees only successfully allocated pools.

2. **Session pool may be undersized (line 218)**
   The session pool is created with size `64`:
   ```c
   env.session_pool = rte_cryptodev_sym_session_pool_create("WYCHEPROOF_SESSION_POOL",
       64, session_size, 0, 0, rte_socket_id());
   ```
   However, the code processes multiple vectors in parallel (potentially one session per vector) and the hash device may need additional sessions. If processing many files with concurrent operations, 64 sessions may be insufficient, causing session creation to fail during validation.
   
   **Recommendation**: Document the concurrency model or increase the pool size to handle worst-case usage (separate sessions for main device + hash device + asymmetric operations).

3. **Hash device selection race condition (lines 253-283)**
   The hash device selection loop configures, sets up queue pairs, and starts a secondary device without checking if it's already in use by another thread or process. If the example is run multiple times concurrently, they may both attempt to configure the same hash device.
   
   **However**, DPDK applications typically run with exclusive device ownership, and the `rte_cryptodev_configure()` would fail if the device is already configured, so this is handled by the existing error checks.

4. **Missing error check in `normalize_operand()` call (line 1652)**
   ```c
   if (normalize_operand(priv, priv_len, priv_buf, bytesize) != 0)
       return -EINVAL;
   ```
   This correctly checks the return value. No issue.

5. **`dequeue_one()` timeout may be too short for slow devices (line 518)**
   The timeout is hardcoded to 1 second (`rte_get_timer_hz()`). For hardware crypto devices under heavy load or debugging scenarios, this may cause spurious timeouts.
   
   **Recommendation**: Make the timeout configurable or increase it to 10+ seconds for robustness.

### Warnings

1. **`hash_dev_own` flag not protected against double-close (lines 303-306)**
   If `app_uninit()` is called twice (e.g., from an atexit handler and explicit call), the hash device would be stopped and closed twice:
   ```c
   if (env.hash_dev_own) {
       rte_cryptodev_stop(env.hash_dev_id);
       rte_cryptodev_close(env.hash_dev_id);
   }
   ```
   **Recommendation**: Set `env.hash_dev_own = false` after closing to prevent double-close.

2. **CCM AAD offset calculation may overflow (line 567)**
   ```c
   aad = rte_zmalloc(NULL,
       RTE_ALIGN_CEIL(vector->aad_len + AES_CCM_AAD_OFFSET, 16), 0);
   ```
   If `vector->aad_len` is close to `UINT32_MAX`, adding `AES_CCM_AAD_OFFSET` could overflow. The `RTE_ALIGN_CEIL` would then produce a small value, leading to a buffer overflow when copying AAD.
   
   **Recommendation**: Check `vector->aad_len + AES_CCM_AAD_OFFSET < UINT32_MAX` before allocation.

3. **Statistics counters use `uint64_t` - should verify no overflow in extreme cases (line 63)**
   The `app_stats` structure uses `uint64_t` for counters. With 2^64 test vectors, overflow is theoretically possible but practically impossible.
   
   **No action needed** - this is acceptable.

## C Coding Style Issues

### Errors

1. **Implicit comparison against NULL/0 in boolean context (multiple locations)**
   Per DPDK style, explicit comparisons are required:
   
   Line 197: `if (env.mbuf_pool == NULL)` - **Correct**
   
   Line 219: `if (env.session_pool == NULL)` - **Correct**
   
   Line 228: `if (env.op_pool == NULL)` - **Correct**
   
   All comparisons in the patch are explicit. **No issues found**.

2. **Use of `bool` type (line 55)**
   ```c
   bool debug;
   bool hash_dev_own;
   ```
   This is correct usage of `bool` for true/false flags, following the guidelines.

### Warnings

1. **Long lines in `process_file()` function (lines 1930-2000)**
   Several lines exceed 100 characters, particularly the algorithm name comparisons:
   ```c
   } else if (strcmp(algorithm, "CHACHA20-POLY1305") == 0 &&
           strcmp(schema, "aead_test_schema_v1.json") == 0) {
   ```
   While meson files have no strict limit, C code should aim for 100 characters or less.
   
   **Recommendation**: Split long conditional chains into helper functions or use line breaks more aggressively.

2. **Complex nested conditions in vector validation (lines 851-895)**
   The `validate_aead_vector()` function has deeply nested if-else chains that are hard to follow. Consider early returns to reduce nesting.
   
   **This is acceptable** for a validation tool where the logic needs to distinguish many cases, but could be refactored for clarity.

## API and Documentation Issues

### Errors

None found. The documentation in `wycheproof_validation.rst` correctly describes the example's purpose and usage.

### Warnings

1. **Release notes claim "covering AEAD, MAC, DSA, ECDH and ECDSA" but SM4-GCM support is not highlighted (line 61)**
   SM4-GCM is mentioned in the commit message and implemented in the code, but the release notes say "AEAD" generically. This is acceptable since SM4-GCM is an AEAD algorithm.

2. **Documentation does not mention Jansson dependency (line 45)**
   The usage section should mention that Jansson is required and how to install it.
   
   **Recommendation**: Add a "Dependencies" section mentioning Jansson.

## Summary

### Errors: 0

No correctness bugs requiring fixes were found.

### Warnings: 3

1. **`hash_dev_own` double-close protection**: Set flag to false after closing hash device in `app_uninit()`.

2. **CCM AAD overflow check**: Validate `vector->aad_len + AES_CCM_AAD_OFFSET` does not overflow before `rte_zmalloc()`.

3. **Documentation completeness**: Add Jansson dependency to user guide.

### Overall Assessment

This is a **well-written validation tool** with careful error handling throughout. The code follows DPDK style conventions correctly (explicit NULL/0 comparisons, proper use of `bool`, appropriate resource cleanup). The main areas for improvement are defensive checks for integer overflow and documentation completeness. No critical correctness bugs were identified.