|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

## Errors

### 1. Resource Leak on Error Path (Line 197-200)
**Session Pool Creation Error Path Leaks Resources**

The error path after `rte_cryptodev_sym_session_pool_create()` fails jumps to the `error:` label, which frees `env.session_pool`, but `env.session_pool` is NULL at this point. The preceding allocations (`env.mbuf_pool`) are not freed.

```c
/* Line 196-200 */
env.session_pool = rte_cryptodev_sym_session_pool_create(...);
if (env.session_pool == NULL) {
    ret = -rte_errno;
    goto error;  /* env.mbuf_pool not freed */
}
```

The error-label code (lines 298-303) attempts to free pools that may be NULL, but the issue is that on this path `env.mbuf_pool` is non-NULL and allocated, yet never freed because the code jumps to `error:` which only frees pools that were successfully created *after* this point.

**Suggested Fix:**
Ensure all successfully allocated resources are freed on all error paths. Either add explicit cleanup before `goto error`, or ensure the error label can handle partial initialization:

```c
error:
    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);  /* Always safe (handles NULL) */
    /* ... rest of cleanup ... */
```

---

### 2. `rte_malloc()` Used for General Allocations (Multiple Locations)
**Inappropriate Use of Hugepage Memory for Non-DMA Buffers**

Throughout the code, `rte_malloc()` and `rte_zmalloc()` are used for general-purpose allocations (decoded hex strings, temporary buffers, digest storage) that are never accessed by DMA and do not need to be in hugepage memory.

Examples:
- Line 371: `decoded = rte_malloc(NULL, *length, 0);` for hex decoding
- Line 571: `*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);` for digest buffer
- Line 624: `aad = rte_zmalloc(NULL, ...)` for AAD (but this one is used with `phys_addr`, so hugepage is correct)
- Line 768: `*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);` (HMAC digest)
- Line 875: `*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);` (GMAC digest)

Only buffers assigned to `phys_addr` fields (lines 625-626, 637-638 in `run_aead`, lines 782-783 in `run_hmac`, lines 896-897 in `run_gmac`) require `rte_malloc()`. Temporary output buffers and decoded hex strings should use standard `malloc()`.

**Suggested Fix (representative example for hex decode):**
```c
/* Line 371 - hex decoding does not need hugepage memory */
decoded = malloc(*length);
if (decoded == NULL)
    return -ENOMEM;
/* ... later ... */
free(decoded);  /* instead of rte_free() */
```

Apply the same change to digest buffers and other non-DMA allocations. Keep `rte_malloc()` only for buffers whose physical address is passed to hardware (those assigned to `.phys_addr` fields).

---

### 3. Missing Error Propagation on Session Create Failure (Line 1265-1270)
**Session Create Failure Returns Generic `-ENOMEM` Instead of Checking Actual Error**

```c
/* Line 1265-1270 */
if (rte_cryptodev_asym_session_create(env.dev_id, &xform, env.asym_session_pool,
        &session) < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}
ret = -ENOMEM;  /* Unconditional after successful session create */
```

The code sets `ret = -ENOMEM` unconditionally after the session-create check, even when the session was created successfully. This is dead-store (the value is overwritten by line 1281 `ret = 0;` on success), but the pattern suggests the intent was to set `-ENOMEM` *before* the check as a default error code. However, the actual error from `rte_cryptodev_asym_session_create()` is lost.

**Suggested Fix:**
Initialize `ret` before the check, not after:

```c
ret = -ENOMEM;  /* Default error code */
if (rte_cryptodev_asym_session_create(..., &session) < 0 || session == NULL) {
    ret = -ENOTSUP;  /* Treat as capability mismatch */
    goto out;
}
/* ret is still -ENOMEM here if alloc within session_create failed */
op = rte_crypto_op_alloc(...);
if (op == NULL)
    goto out;
/* ... rest of function ... */
```

Or, better: capture the return value from `rte_cryptodev_asym_session_create()` and propagate it if it's not a capability issue.

**Same pattern appears at:**
- Line 1545 (`run_ecdh_ecpoint`)
- Line 1763 (`run_ecdsa_verify`)

---

### 4. Unbounded Descriptor Chain Traversal Risk (Lines 2076-2093)
**Directory Traversal Without Bounds Check on Path Construction**

```c
/* Line 2076-2093 */
while ((entry = readdir(directory)) != NULL) {
    char file_path[PATH_MAX];
    size_t name_length = strlen(entry->d_name);

    if (name_length < 6 || strcmp(entry->d_name + name_length - 5, ".json") != 0)
        continue;
    if (snprintf(file_path, sizeof(file_path), "%s/%s", path, entry->d_name) >=
            (int)sizeof(file_path)) {
        ret = -ENAMETOOLONG;
        break;
    }
    /* ... */
}
```

This is not technically an unbounded descriptor chain (it's a directory traversal), but the bounds check at line 2083 happens *after* the `snprintf()` has already been called. While `snprintf()` itself is safe, this is the correct pattern, so this is actually **not an error**. However, there's a subtle integer-promotion issue.

**Actually, this code is correct.** The check `>= (int)sizeof(file_path)` correctly detects truncation. Retract this item.

---

### 5. Variable Overwrite Before Read (Lines 1265-1270, Repeated)
**Already covered in Error #3 above.**

---

## Warnings

### 1. Queue-Related Buffer Not Using `rte_zmalloc_socket()` (Line 194)
**Mbuf Pool Not NUMA-Local**

```c
/* Line 194 */
env.mbuf_pool = rte_pktmbuf_pool_create("WYCHEPROOF_MBUF_POOL", 64, 0, 0,
    env.mbuf_data_room, rte_socket_id());
```

While `rte_pktmbuf_pool_create()` internally allocates from hugepages and respects the socket argument, this warning is **not applicable** to mempool creation APIs (they already handle NUMA). Retract this item.

---

### 2. Hardcoded Overhead Constant (Line 34)
**AES-CCM AAD Offset Hardcoded Instead of Derived from Spec**

```c
#define AES_CCM_AAD_OFFSET 18
```

This is a fixed protocol constant (CCM B0 block size + length field), not a device-dependent overhead. This is correct. Retract this item.

---

### 3. Missing Release Notes for Internal Changes
**No release notes entry for ... (reviewing)**

The patch includes release notes (lines 51-58 in `release_26_11.rst`), which correctly describe the new example application. Release notes are present and appropriate. No issue.

---

### 4. `bool` Type Not Used for Flags (Line 53, 60)
**Struct Fields Use `bool` Appropriately**

```c
struct app_env {
    /* ... */
    bool hash_dev_own;  /* Line 53 */
    /* ... */
    bool debug;         /* Line 60 */
};
```

The code already uses `bool` for flag fields. No issue.

---

### 5. Unchecked Return Value from `pthread_*()` Functions
**Scanning for pthread calls... None found.** The code uses DPDK's threading and synchronization primitives (`rte_cryptodev_*`), not raw `pthread_*()`. No issue.

---

### 6. Missing Functional Test Integration
**Example Does Not Integrate with `app/test` Suite**

This is an **example application**, not a library or driver. Examples are not required to have unit tests in `app/test`. The application itself is a validation tool. No issue.

---

### 7. Non-const Function Pointer Array (Checking...)
**No function pointer arrays found in the patch.** The patch uses direct function calls, not dispatch tables. No issue.

---

### 8. `int` Used Instead of `bool` for Predicate Return (Lines 1412-1417)
**`value_equals_padded()` Should Return `bool`**

```c
static bool
value_equals_padded(const uint8_t *a, uint32_t a_len, const uint8_t *b, uint32_t b_len)
{
    /* ... */
    return a_len == b_len && memcmp(a, b, a_len) == 0;
}
```

This function already returns `bool`. No issue.

---

### 9. Statistics Accumulation Using `=` Instead of `+=`
**All Statistics Updates Use `++` (increment by 1)**

Lines 702, 727, 734, etc. all use `stats->passed++`, `stats->failed++`, which is correct accumulation. No issue.

---

### 10. Standalone Makefile Build Flags
**Makefile Uses Correct LDFLAGS Pattern**

Lines 24-25 separate `LDFLAGS_SHARED` and `LDFLAGS_STATIC`. This is standard practice for DPDK examples. No issue.

---

## Summary

### Errors (must fix):
1. **Resource leak on error path** after `rte_cryptodev_sym_session_pool_create()` fails (line 197-200) -- `env.mbuf_pool` not freed
2. **Inappropriate use of `rte_malloc()`** for non-DMA buffers (hex decode line 371, digest buffers lines 571, 768, 875, etc.) -- should use standard `malloc()`
3. **Dead store / misleading error code** in `run_dsa_verify()` line 1270 (and similar in `run_ecdh_ecpoint` line 1545, `run_ecdsa_verify` line 1763) -- `ret = -ENOMEM;` set after successful session create, never read

### Warnings:
None (all initially flagged items were either correct code or not applicable to example applications).

### Info:
The code is generally well-structured and follows DPDK conventions for an example application. The main issues are correctness bugs around resource management and memory allocation choice.