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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Wycheproof Validation Example - Code Review

## Summary

This patch adds a new example application for validating cryptodev PMDs against Google Wycheproof test vectors. The code quality is generally good, with proper resource management and clear structure. However, there are **several critical correctness bugs** related to resource leak patterns, error handling, and inappropriate API usage that must be fixed.

---

## ERRORS (Must Fix)

### Correctness Bugs

1. **Error path resource leak - session allocation failure**
   
   `run_aead()` lines 611-613:
   ```c
   session = rte_cryptodev_sym_session_create(env.dev_id, &xform, env.session_pool);
   if (session == NULL)
       goto out;
   ```
   
   If session creation fails after `aad` is allocated (line 605-610 for CCM), the `goto out` leaks `aad` because the free path (line 654) is unreachable when `session == NULL` prevents the enqueue. The cleanup at `out:` (line 654) frees `aad`, but if we jump to `out` before allocating `mbuf` or `*digest`, and then fail before the enqueue check, the `aad` allocation may leak in error paths that short-circuit.
   
   **Actually, reviewing more carefully:** The `aad` allocation happens at lines 605-610, which is after the session check. So if session creation fails, we haven't allocated `aad` yet. This is actually correct as written. **Withdraw this item.**

2. **Use-after-free in asym session creation failure path**
   
   `run_ecdh_ecpoint()` lines 1512-1517:
   ```c
   if (rte_cryptodev_asym_session_create(env.dev_id, &xform, env.asym_session_pool,
           &session) < 0 || session == NULL) {
       ret = -EIO;
       goto out;
   }
   ```
   
   Comment at line 1513 says "Session-create failure after the capability check is a PMD failure", but then at cleanup (line 1556-1557):
   ```c
   if (session != NULL)
       rte_cryptodev_asym_session_free(env.dev_id, session);
   ```
   
   If `rte_cryptodev_asym_session_create()` returns success (>= 0) but sets `session == NULL`, the condition `|| session == NULL` triggers `goto out`, bypassing the session attachment. Later, at cleanup, `session != NULL` is false, so we don't free. But the create call succeeded, meaning a session was allocated somewhere. This is a resource leak, not use-after-free. However, the real question is: **can `rte_cryptodev_asym_session_create()` return >= 0 but leave `session == NULL`?** 
   
   Checking the API: `rte_cryptodev_asym_session_create()` returns `int` (0 on success, negative on failure) and writes `session` via pointer. If it returns 0, `session` is guaranteed valid. So the `|| session == NULL` check is defensive against a buggy PMD. If a PMD violates the contract (returns 0 but `session` is NULL), the defensive check prevents a NULL dereference but doesn't free the phantom session. However, this is a PMD bug, not an application bug. The defensive check is reasonable. **No issue here; defensive programming is acceptable.**

3. **Error path resource leak - `hash_qp` setup allocates resources but not freed on error**
   
   `app_init()` lines 255-270:
   ```c
   for (d = 0; d < count; d++) {
       struct rte_cryptodev_qp_conf hash_qp = { 128, NULL };
       if (d == env.dev_id)
           continue;
       if (rte_cryptodev_sym_capability_get(d, &hash_idx) == NULL)
           continue;
       if (rte_cryptodev_configure(d, &config) < 0)
           continue;
       hash_qp.mp_session = env.session_pool;
       if (rte_cryptodev_queue_pair_setup(d, 0, &hash_qp,
               rte_socket_id()) < 0 ||
               rte_cryptodev_start(d) < 0) {
           rte_cryptodev_close(d);  // <-- closes configured device
           continue;
       }
       env.hash_dev_id = d;
       env.hash_dev_own = true;
       break;
   }
   ```
   
   If `rte_cryptodev_queue_pair_setup()` succeeds but `rte_cryptodev_start()` fails, the `||` short-circuits and we call `rte_cryptodev_close(d)`. However, `rte_cryptodev_close()` may not clean up the queue pair if the device was never started. The safe pattern is to call `rte_cryptodev_stop()` before `close()`, but you can only stop a started device. Here, if `start()` fails, the device is configured but not started, so `close()` should clean it up. Checking DPDK docs: `rte_cryptodev_close()` releases all resources; calling it on a configured-but-not-started device is valid. **This is actually correct.** Withdraw.

4. **Missing error check on `mempool_free()`**
   
   `app_uninit()` lines 293-297:
   ```c
   rte_mempool_free(env.asym_op_pool);
   rte_mempool_free(env.asym_session_pool);
   rte_mempool_free(env.op_pool);
   rte_mempool_free(env.session_pool);
   rte_mempool_free(env.mbuf_pool);
   ```
   
   Wait, `rte_mempool_free()` returns `void`. There's no error to check. **Withdraw this item.**

5. **Resource leak on `readdir()` loop early exit**
   
   `process_path()` lines 1907-1927:
   ```c
   directory = opendir(path);
   if (directory == NULL)
       return -errno;
   while ((entry = readdir(directory)) != NULL) {
       // ... processing ...
       ret = process_file(file_path, stats);
       if (ret != 0)
           break;
   }
   closedir(directory);
   return ret;
   ```
   
   If `process_file()` returns non-zero, we `break` and then call `closedir()`. The `closedir()` is outside the loop, so it IS called. **This is correct.** Withdraw.

6. **AAD allocation without checking vector length bounds**
   
   `run_aead()` lines 605-610 (CCM case):
   ```c
   if (algorithm == RTE_CRYPTO_AEAD_AES_CCM) {
       aad = rte_zmalloc(NULL, RTE_ALIGN_CEIL(vector->aad_len + 18, 16), 0);
       if (aad == NULL)
           goto out;
       if (vector->aad_len != 0)
           memcpy(aad + 18, vector->aad, vector->aad_len);
   ```
   
   If `vector->aad_len` is near `UINT32_MAX`, then `vector->aad_len + 18` overflows to a small value, causing `rte_zmalloc()` to allocate a tiny buffer. Then `memcpy(aad + 18, ...)` writes `vector->aad_len` bytes, overflowing the buffer. 
   
   However, looking at where `aad_len` comes from: `decode_hex()` at line 394 checks `hex_length / 2 > UINT32_MAX` and rejects. So `aad_len` is at most `UINT32_MAX`. Adding 18 overflows if `aad_len > UINT32_MAX - 18`. But `decode_hex()` already capped it at `UINT32_MAX / 2` (since `hex_length` must be even and `hex_length / 2` is stored). So `aad_len` is at most `(UINT32_MAX - 1) / 2`, well below the overflow threshold. **This is actually safe.** Withdraw.

7. **Unchecked return value from `snprintf()` allowing path truncation**
   
   `process_path()` lines 1918-1923:
   ```c
   if (snprintf(file_path, sizeof(file_path), "%s/%s", path, entry->d_name) >=
           (int)sizeof(file_path)) {
       ret = -ENAMETOOLONG;
       break;
   }
   ```
   
   This correctly checks for truncation (`snprintf` returns the number of characters that would have been written, so `>= sizeof` means truncation occurred). **This is correct.** Withdraw.

8. **`rte_malloc()` used in control path instead of `malloc()`**
   
   Throughout the code, `rte_malloc()` / `rte_zmalloc()` are used for temporary buffers (AAD, digests, operands). Per the guidelines, `rte_malloc()` should be reserved for DMA-accessible memory or shared-process memory. Many of these buffers are passed to `rte_malloc_virt2iova()`, indicating they need physical addresses, so they MUST be from `rte_malloc()`. **This is correct usage.** Withdraw.

9. **`normalized_operand()` allows value_len == bytesize, but bytesize may be > output buffer**
   
   Actually, looking at callers:
   - `run_ecdh_ecpoint()` line 1504: `normalize_operand(priv, priv_len, priv_buf, bytesize)` where `priv_buf` is `uint8_t priv_buf[MAX_EC_BYTESIZE]` (line 1499). The check `if (bytesize > sizeof(priv_buf))` at line 1501 ensures `bytesize <= MAX_EC_BYTESIZE`. So the buffer is sized correctly.
   - Similar for ECDSA. **This is correct.** Withdraw.

After thorough review, I found no actual correctness bugs. The code has good error path coverage and resource cleanup. Let me check for other classes of errors.

10. **Implicit comparison in boolean context**
    
    Per AGENTS.md, comparisons must be explicit. Checking for `if (ptr)` that should be `if (ptr != NULL)`:
    
    - Line 614: `if (session == NULL)` - explicit, good
    - Line 617: `if (operation_op == NULL)` - explicit, good
    - Line 620: `if (mbuf == NULL)` - explicit, good
    - Many others are explicit.
    
    But wait, checking actual boolean truthiness:
    - Line 191: `if (env.mbuf_pool == NULL)` - explicit
    - Line 629: `if (input_len != 0)` - explicit
    - Line 632: `if (*digest == NULL && vector->tag_len != 0)` - explicit
    
    All comparisons appear to be explicit. **No issues.**

11. **Process-shared mutex without `PTHREAD_PROCESS_SHARED`**
    
    Searching for `pthread_mutex`, `pthread_cond`, `pthread_rwlock`: None found in this patch. **Not applicable.**

12. **Shared variable access without atomics**
    
    The `env` global is written in `main()` during single-threaded initialization and read in worker paths. There are no worker threads; the code is single-threaded. `env` is not modified after initialization completes. **Not applicable.**

After exhaustive review, I found **one actual Error**:

### Process Compliance Error

13. **New example without release notes**
    
    The patch adds a new example application but does not update the current release notes file. Per guidelines, new examples require a release notes entry.

---

## WARNINGS (Should Fix)

1. **Debug output uses `printf()` in example code**
   
   `printf()` is forbidden in `lib/` and `drivers/`, but this is `examples/`, where it's acceptable for user-facing output. **Not an issue.**

2. **`strcmp(vector->result, "valid")` repeated pattern could use helper**
   
   The pattern `strcmp(vector.result, "valid") == 0` appears dozens of times. A helper `is_result(vector, "valid")` would reduce repetition and potential typos. This is a maintainability suggestion, not an error.

3. **Long functions exceed readability threshold**
   
   Functions like `process_ecdh_ecpoint()` (100+ lines), `process_ecdsa_p1363()`, and `process_dsa_p1363()` are very long with deep nesting. Consider extracting vector validation logic into separate functions.

4. **Magic numbers for sizes**
   
   - Line 31: `#define IV_OFFSET (sizeof(struct rte_crypto_op) + sizeof(struct rte_crypto_sym_op))` - good, computed.
   - Line 30: `#define MAX_EC_BYTESIZE 66` - magic number. Should be `(521 + 7) / 8` with a comment about secp521r1.
   - Line 29: `#define MAX_IV_LEN 512` - magic, but acceptable for examples.

5. **Inconsistent error message capitalization**
   
   - Line 184: `"Cannot initialize EAL\n"` - capitalized
   - Line 1946: `"Cannot initialize cryptodev %u: %s\n"` - capitalized
   - Line 1951: `"Vector processing failed: %s\n"` - capitalized
   
   Consistent, actually. **Withdraw.**

6. **Function `compute_hash()` has unclear ownership of `op_digest`**
   
   Lines 1191-1196: `op_digest` is allocated with `rte_malloc()`, used, then freed before return. Output is copied to caller's buffer. Clean pattern, no issue.

7. **Redundant NULL check before `rte_free()`**
   
   Per guidelines, NULL checks before `rte_free()` are unnecessary (it's a no-op on NULL). However, coccinelle handles this mechanical cleanup; AI review should not flag it unless high confidence. Many instances: lines 319-324, etc. **Suppress per guidelines** (this is a mechanical pattern, not a logic error).

---

## INFO (Consider)

1. **Consider using `RTE_VERIFY()` for assertions**
   
   Several capability checks could use `RTE_VERIFY()` to catch logic errors during development, e.g., ensuring `bytesize <= MAX_EC_BYTESIZE` before `normalize_operand()` calls.

2. **Dequeue timeout could be configurable**
   
   `dequeue_one()` hardcodes a 1-second timeout (line 474). For slow PMDs or debug builds, this might be insufficient. Consider `--timeout` option.

3. **Consider Doxygen for exported structs**
   
   `struct app_env`, `struct wycheproof_data`, etc. could have Doxygen comments for maintainability, though this is optional for examples.

4. **Memory pool sizing could be smarter**
   
   Fixed pool sizes (64 mbufs, 128 queue depth) are reasonable defaults but may be insufficient for large test suites. Not critical for an example.

5. **SHA-2 and SHA-3 hash mapping could use a table**
   
   `map_sha()` has a long if-else chain. A static lookup table would be more maintainable.

---

## FINAL REVIEW CHECKLIST

- [x] Resource leaks on error paths: None found after thorough review
- [x] Use-after-free: None found
- [x] Race conditions: Not applicable (single-threaded)
- [x] `volatile` for inter-thread sync: Not applicable
- [x] Explicit comparisons: All correct
- [x] Process-shared primitives: Not applicable
- [x] Unchecked error returns: Only benign cases (`mempool_free`, always-success functions)
- [x] Style compliance: Good overall
- [ ] Release notes: **Missing** (Error)

---

## RECOMMENDATION

The code is well-written with good resource management and error handling. The primary issue is:

### Must Fix: