|WARNING| examples: add Wycheproof validation app

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: Wycheproof Validation Example

## Summary

This patch adds a comprehensive Wycheproof JSON vector validation example for cryptodev PMDs. The implementation is generally sound with good error handling, but has several correctness bugs related to resource management and memory safety, plus some style issues.

---

## Errors (Must Fix)

### Resource Leaks

**Error:** Missing resource cleanup on `app_init()` error paths (lines 274-284)

The asymmetric pools (`asym_session_pool`, `asym_op_pool`) are created but not freed when subsequent operations fail (queue setup or device start). The code jumps to `error:` label which only frees the earlier pools.

```c
/* Current code - leaks asym pools on error */
env.asym_session_pool = rte_cryptodev_asym_session_pool_create(...);
env.asym_op_pool = rte_crypto_op_pool_create(...);

queue_pair.mp_session = env.session_pool;
ret = rte_cryptodev_queue_pair_setup(env.dev_id, 0, &queue_pair,
    rte_socket_id());
if (ret < 0)
    goto error;  /* BUG: asym pools not freed */
```

**Fix:** Add the asym pools to the cleanup label or add explicit cleanup before goto.

---

**Error:** Resource leak in `run_dsa_verify()` on operation path (lines 1153-1159)

The DSA verify function allocates a session and an op, but if `rte_cryptodev_enqueue_burst()` fails, only the op is freed at the `out:` label. The session remains allocated.

Actually, reviewing the cleanup at `out:` (lines 1167-1170), the session *is* freed when non-NULL. The issue is that if `rte_crypto_op_alloc()` fails, we `goto out` with `session` potentially non-NULL from the earlier `rte_cryptodev_asym_session_create()` call. This is correct.

However, reviewing more carefully: if session creation succeeds but op allocation fails, we goto out with session != NULL, which correctly frees it. If enqueue fails, we also goto out, which frees the op then the session. This appears correct on closer inspection.

**Withdraw this item** - the cleanup logic is correct.

---

**Error:** Resource leak in `run_ecdh_ecpoint()` on early session create failure (lines 1331-1339)

When `rte_cryptodev_asym_session_create()` fails, the function sets `ret = -ENOTSUP` and jumps to `out:`. At the `out:` label (lines 1368-1373), `out_x` and `out_y` are freed, but they were allocated at lines 1335-1336. If the session create fails *before* these allocations (which it does - session create is at lines 1337-1339, allocations at 1335-1336), then we attempt to free uninitialized pointers.

Wait, re-reading: `out_x` and `out_y` are declared at line 1315 but not initialized to NULL. Then at lines 1335-1336:

```c
out_x = rte_zmalloc(NULL, bytesize, 0);
out_y = rte_zmalloc(NULL, bytesize, 0);
if (out_x == NULL || out_y == NULL)
    goto out;
```

So if allocation fails, we goto out, and the cleanup does `rte_free(out_x); rte_free(out_y);` - but if one is NULL and the other isn't, we leak the non-NULL one. Actually, `rte_free(NULL)` is safe, so if `out_x` is non-NULL but `out_y` is NULL, we goto out and free `out_x` then `out_y` (which is NULL, safe), then free op (which is NULL), then free session (which is NULL). This is correct.

**Withdraw this item** - the logic is correct; `rte_free(NULL)` is safe.

---

**Error:** Resource leak in `compute_hash()` when mbuf allocation fails (line 1091)

If `rte_pktmbuf_alloc()` returns NULL, the function jumps to `out:` without freeing the session that was successfully created earlier. The session is freed at `out:` (line 1113), so this is actually correct.

**Withdraw this item** - cleanup is correct.

---

### Use-After-Free / Dangling Pointer

**Error:** Potential use of freed memory in `run_aead()` error path (lines 568-570)

If any allocation fails after `session` is created but before mbuf allocation succeeds, the code jumps to `out:` where it calls `rte_cryptodev_sym_session_free(env.dev_id, session)`. However, the session was attached to `operation_op` if that was successfully allocated. The `rte_crypto_op_free(operation_op)` at line 569 may access the freed session.

Actually, reviewing the code: `rte_crypto_op_attach_sym_session(operation_op, session)` is called at line 551, which is *after* all allocations succeed and before enqueue. So if we jump to `out:` before line 551, `operation_op` has no attached session and freeing the op is safe. The session free at line 570 comes after the op free at line 569, so the op is freed first (which is safe even if it has an attached session pointer, as op free doesn't touch the session), then the session is freed.

**Withdraw this item** - the order is safe.

---

### Missing NULL Checks

**Error:** Missing NULL check after `rte_malloc()` calls for digest/AAD buffers in multiple functions

Example at line 539 in `run_aead()`:

```c
*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);
if (*digest == NULL && vector->tag_len != 0)
    goto out;
```

This check has the pattern `if (ptr == NULL && size != 0)`. This means when `tag_len == 0`, the NULL check is skipped. However, immediately after (line 541), the code does:

```c
if (operation == RTE_CRYPTO_AEAD_OP_DECRYPT && vector->tag_len != 0)
    memcpy(*digest, vector->tag, vector->tag_len);
```

So if `tag_len == 0`, we skip both the NULL check and the memcpy. The `*digest` pointer (which is NULL) is then assigned to `sym_op->aead.digest.data` at line 557. Passing a NULL digest pointer to the PMD when the digest length is zero is likely acceptable (PMDs should handle zero-length digests), but the subsequent `rte_malloc_virt2iova(*digest)` at line 558 will pass NULL to `rte_malloc_virt2iova`, which may be undefined.

Checking `rte_malloc_virt2iova` documentation: it returns `RTE_BAD_IOVA` on error, but passing NULL is likely considered an error case. However, many PMDs likely ignore the digest physical address when digest length is zero.

This pattern repeats in:
- `run_aead()` line 539 (digest)
- `run_aead()` line 545 (aad for non-CCM)
- `run_hmac()` line 647
- `run_gmac()` line 714
- `compute_hash()` line 1100

**Severity judgment:** This is a corner case where zero-length digests are being processed. While technically calling `rte_malloc_virt2iova(NULL)` is dubious, the zero-length case is likely handled by PMDs. However, the code should either:
1. Always check for NULL after rte_malloc (even when size is 0, to detect allocation failure)
2. Or skip the iova call when the pointer is NULL

**Classification:** Potential bug, but low severity given zero-length digest/AAD is an edge case. Report as **Warning** since it's not a clear-cut use-after-free or leak, but rather a questionable API usage.

---

### Correctness - Signature Verification

**Error:** DSA/ECDSA signature split assumes even length without checking alignment (lines 1219, 1596)

```c
if ((sig_len & 1) != 0 || sig_len == 0) {
    debug_vector("SKIP", name, &vector, "malformed P1363 signature");
    stats->skipped_unsupported++;
    rte_free(vector.msg);
    rte_free(sig);
    continue;
}
ret = run_dsa_verify(&dsa, vector.msg, vector.msg_len, sig, sig_len / 2,
    sig + sig_len / 2, sig_len / 2, &status);
```

The code correctly checks that `sig_len` is even and non-zero before splitting the signature into `r` and `s` components. This is correct P1363 encoding (fixed-width `r || s`).

**Withdraw this item** - the check is correct.

---

## Warnings (Should Fix)

### Style and Coding Convention

**Warning:** Implicit NULL comparison on line 119

```c
if (rte_cryptodev_count() == 0)
```

DPDK style requires explicit comparison against zero for integer values. Should be:

```c
if (rte_cryptodev_count() == 0)  /* This is already explicit */
```

Actually, this is already correct - the comparison `== 0` is explicit.

---

**Warning:** Multiple functions lack blank line between declarations and statements

Examples:
- Line 1089 in `compute_hash()`: `uint8_t *op_digest = NULL;` followed immediately by `capability = ...`
- Line 1137 in `run_dsa_verify()`: `uint8_t digest[64];` followed immediately by `if (env.asym_session_pool == NULL ...`

DPDK style requires an empty line between variable declarations and the first statement.

---

**Warning:** Zero-length buffer allocation checks inconsistent

In multiple functions, the pattern:

```c
*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);
if (*digest == NULL && vector->tag_len != 0)
    goto out;
```

This means when `tag_len == 0`, a NULL allocation result (which `rte_malloc` returns for zero-size) is silently accepted. The code then calls `rte_malloc_virt2iova(*digest)` on this NULL pointer. While zero-length digests may be valid for some algorithms, calling iova on NULL is questionable. Consider either:

1. Skip the iova call when length is zero
2. Or allocate a minimum-size buffer even for zero-length (wasteful but simpler)

---

**Warning:** Missing `const` qualifier for several read-only parameters

Functions that take pointers to data that is never modified should declare those parameters `const`:

- `compute_hash()` parameter `msg` (line 1078)
- `run_dsa_verify()` parameters `msg`, `r`, `s` (line 1130)
- `run_ecdh_ecpoint()` parameters `priv`, `pub_x`, `pub_y` (line 1301)
- `run_ecdsa_verify()` parameters `msg`, `r`, `s` (line 1502)

---

**Warning:** Line length violations

Multiple lines exceed the 100-character soft limit:

- Line 70 (140 characters)
- Line 123 (long while loop condition)
- Line 1091 (long if condition)
- Many others in the JSON parsing sections

Consider breaking these into multiple lines or shorter variable names.

---

**Warning:** Inconsistent error message formatting

Some error messages use `printf()` while others are silent (relying only on debug mode). For example:

- Line 1837: `printf("Cannot initialize cryptodev %u: %s\n", ...)`
- Line 1844: `printf("Vector processing failed: %s\n", ...)`

But many error paths are completely silent unless `env.debug` is set. Consider consistent error reporting for user-facing failures (file not found, cryptodev init failure, etc.) vs. debug-only reporting for per-vector failures.

---

### Documentation and API

**Warning:** README.md uses relative URLs for Wycheproof vectors

Lines 105-118 in README.md reference `../wycheproof/testvectors_v1/` which assumes a specific directory layout. Users may have cloned Wycheproof elsewhere. Consider clarifying that users need to download Wycheproof separately and adjust the path, or provide a note about where to obtain it.

---

**Warning:** No release notes update

This is a new example application with new build dependencies (Jansson) and new functionality. It should have a release notes entry in `doc/guides/rel_notes/release_*.rst` documenting:

- New example: wycheproof_validation
- Purpose: Validate DPDK cryptodev PMDs against Wycheproof test vectors
- Dependencies: Requires Jansson library
- Supported algorithms: List of AEAD, MAC, and asymmetric algorithms

---

### Resource Management

**Warning:** Asymmetric pools created unconditionally but may not be used

Lines 221-224 create `asym_session_pool` and `asym_op_pool` even when the target device may not support asymmetric crypto. The comment at line 219 says these are "best-effort", but they consume memory even when unused.

Consider deferring creation until actually needed (lazy initialization on first asymmetric operation), or check device capabilities before creating.

---

**Warning:** `hash_dev_id` selection logic could be clearer

Lines 249-271 implement a fallback mechanism to find a separate symmetric-capable device for hashing when the main device is asymmetric-only. The logic is correct but complex. Consider:

1. Extracting this into a separate function `find_hash_device()`
2. Adding a comment explaining why this is needed (QAT asym-only devices)
3. Documenting what happens if no hash-capable device is found (currently, DSA/ECDSA will fail at runtime with -ENOTSUP)

---

## Info (Consider)

### Optimization

**Info:** Repeated `strcmp()` calls in dispatch logic (lines 1679-1783)

The `process_file()` function performs sequential string comparisons for algorithm and schema. For a large batch of files, this could be optimized with a lookup table or switch on the first character followed by more specific checks.

However, given this is a test/validation tool (not fast-path code), the current approach is acceptable for clarity.

---

**Info:** Synchronous crypto operation with busy-wait dequeue

Multiple functions use:

```c
while (rte_cryptodev_dequeue_burst(env.dev_id, 0, &completed, 1) == 0)
    rte_pause();
```

For a validation tool, synchronous operation is appropriate. For higher performance, batching and async operation would help, but this is not the goal of this example.

---

**Info:** Fixed buffer size for digest in `compute_hash()` (line 1137)

```c
uint8_t digest[64];
```

The maximum digest size in the code is 64 bytes (SHA-512). This is safe for all algorithms currently supported, but a comment or constant would make the choice explicit:

```c
uint8_t digest[RTE_CRYPTO_MAX_DIGEST_SIZE];
/* Or: */
#define MAX_DIGEST_SIZE 64  /* SHA-512 */
uint8_t digest[MAX_DIGEST_SIZE];
```

---

### Code Organization

**Info:** Large functions with multiple responsibilities

Functions like `process_file()` (lines 1679-1783) and `process_dsa_p1363()` (lines 1181-1246) are quite long. Consider extracting sub-functions for:

- JSON validation (check algorithm + schema)
- Per-algorithm dispatch logic

This would improve readability and testability.

---

**Info:** Magic number for AAD offset in AES-CCM (line 549)

```c
memcpy(aad + 18, vector->aad, vector->aad_len);
```

The 18-byte prefix is CCM-specific but not explained in code. Add a comment or constant:

```c
#define RTE_CRYPTO_AEAD_AES_CCM_AAD_PREFIX_SIZE 18
memcpy(aad + RTE_CRYPTO_AEAD_AES_CCM_AAD_PREFIX_SIZE, vector->aad, vector->aad_