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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Wycheproof Validation Example - Code Review

## Summary

This patch adds a new example application for validating DPDK cryptodev PMDs against Google Wycheproof test vectors. The code is generally well-structured and follows DPDK conventions, but I've identified several **critical correctness bugs** (resource leaks on error paths) and a few style/process issues.

---

## ERRORS (Must Fix)

### 1. Resource leak on hash device initialization failure (line 288)
**File:** `examples/wycheproof_validation/main.c:288`

When the hash device queue pair setup or start fails inside the loop, `rte_cryptodev_close(d)` is called, but the outer function returns normally without cleaning up the previously allocated `env.session_pool`, `env.op_pool`, and `env.mbuf_pool`. The success path after the loop falls through to `return 0`, leaking these resources when no suitable hash device was found.

```c
/* Current code around line 280 */
if (rte_cryptodev_sym_capability_get(env.dev_id, &hash_idx) == NULL) {
    for (d = 0; d < count; d++) {
        /* ... */
        if (rte_cryptodev_queue_pair_setup(d, 0, &hash_qp,
                rte_socket_id()) < 0 ||
                rte_cryptodev_start(d) < 0) {
            rte_cryptodev_close(d);
            continue;
        }
        env.hash_dev_id = d;
        env.hash_dev_own = true;
        break;
    }
}
return 0;  /* BUG: no cleanup if loop exhausted without finding hash dev */
```

**Fix:** Check whether a hash device was successfully configured when required, and clean up allocated resources if not:

```c
/* After the hash device search loop, before return 0: */
if (rte_cryptodev_sym_capability_get(env.dev_id, &hash_idx) == NULL &&
        !env.hash_dev_own) {
    ret = -ENOTSUP;
    goto error;
}
return 0;
```

---

### 2. Resource leak in `run_aead` on enqueue failure (line 601)
**File:** `examples/wycheproof_validation/main.c:601`

When `rte_cryptodev_enqueue_burst` fails (`ret = -EIO`), the function jumps to `out:`, which frees `aad`, `mbuf`, and `operation_op`, but **does NOT free `session`** or the allocated `digest`. On the error path, `session` should be freed, and the output pointers should be cleared.

```c
if (rte_cryptodev_enqueue_burst(env.dev_id, 0, &operation_op, 1) != 1) {
    ret = -EIO;
    goto out;  /* BUG: session not freed, digest not freed */
}
```

**Fix:** Ensure `session` is freed on all error paths before `out:` label, or restructure the `out:` label to always free the session. Current code only frees session when `ret == 0` implicitly falls through; the label should free it unconditionally:

```c
out:
    rte_free(aad);
    rte_pktmbuf_free(mbuf);
    rte_crypto_op_free(operation_op);
    if (session != NULL)
        rte_cryptodev_sym_session_free(env.dev_id, session);
    if (ret != 0) {
        rte_free(*output);
        rte_free(*digest);
        *output = NULL;
        *digest = NULL;
    }
    return ret;
```

Note: The current code has `rte_cryptodev_sym_session_free(env.dev_id, session);` after the `out:` label (line 625), which is only reached on success. This pattern is wrong -- it should be inside the `out:` block.

---

### 3. Resource leak in `run_hmac` on enqueue failure (line 688)
**File:** `examples/wycheproof_validation/main.c:688`

Same pattern as `run_aead`: `rte_cryptodev_enqueue_burst` failure (`ret = -EIO; goto out;`) does not free `session` or `*digest`. The `out:` label only frees `session` after the label on the normal path, not on error.

**Fix:** Move `rte_cryptodev_sym_session_free` inside the `out:` block, similar to the fix for `run_aead`.

---

### 4. Resource leak in `run_gmac` on enqueue failure (line 760)
**File:** `examples/wycheproof_validation/main.c:760`

Identical to `run_hmac` and `run_aead`: session not freed on enqueue error.

**Fix:** Move `rte_cryptodev_sym_session_free` inside the `out:` block.

---

### 5. Resource leak in `compute_hash` on enqueue failure (line 1217)
**File:** `examples/wycheproof_validation/main.c:1217`

Enqueue failure does not free `session` before `goto out;`.

**Fix:** Restructure `out:` to always free `session`.

---

### 6. Resource leak in `run_dsa_verify` on enqueue failure (line 1279)
**File:** `examples/wycheproof_validation/main.c:1279`

Enqueue failure does not free `session` before `goto out;`.

**Fix:** Move `rte_cryptodev_asym_session_free` into the `out:` block.

---

### 7. Resource leak in `run_ecdh_ecpoint` on enqueue failure (line 1553)
**File:** `examples/wycheproof_validation/main.c:1553`

Enqueue failure does not free `session` before `goto out;`.

**Fix:** Move `rte_cryptodev_asym_session_free` into the `out:` block.

---

### 8. Resource leak in `run_ecdsa_verify` on enqueue failure (line 1810)
**File:** `examples/wycheproof_validation/main.c:1810`

Enqueue failure does not free `session` before `goto out;`.

**Fix:** Move `rte_cryptodev_asym_session_free` into the `out:` block.

---

### 9. Missing error checks on `rte_mempool_free` cleanup calls
**File:** `examples/wycheproof_validation/main.c:310-318`

While `rte_mempool_free` does not return an error (it's `void`), the pointers are not NULL-checked before freeing, and after freeing they are not set to NULL. If the initialization path is partially completed and `app_uninit` is called, a dangling pointer could be freed again. The code does set them to NULL on line 318 after the error path, but this is inside the `error:` label -- on normal cleanup via `app_uninit`, they are not cleared.

**Recommendation:** Set pointers to NULL after `rte_mempool_free` in `app_uninit` as well, or verify that `app_uninit` is never called after a failed `app_init` (which is the case here, but defensive coding is safer).

**Severity:** This is borderline -- the current usage pattern is safe (app_uninit is only called after successful app_init), but the inconsistency is error-prone for future refactoring. Flagging as **Error** because the pattern could lead to double-free if someone reorders the cleanup logic.

**Fix:** In `app_uninit`, set pointers to NULL after freeing:

```c
static void
app_uninit(void)
{
    if (env.hash_dev_own) {
        rte_cryptodev_stop(env.hash_dev_id);
        rte_cryptodev_close(env.hash_dev_id);
    }
    rte_cryptodev_stop(env.dev_id);
    rte_cryptodev_close(env.dev_id);
    rte_mempool_free(env.asym_op_pool);
    env.asym_op_pool = NULL;
    rte_mempool_free(env.asym_session_pool);
    env.asym_session_pool = NULL;
    rte_mempool_free(env.op_pool);
    env.op_pool = NULL;
    rte_mempool_free(env.session_pool);
    env.session_pool = NULL;
    rte_mempool_free(env.mbuf_pool);
    env.mbuf_pool = NULL;
}
```

---

## WARNINGS (Should Fix)

### 1. Missing release notes for new API or test-only code clarification
**File:** `doc/guides/rel_notes/release_26_11.rst:58`

The release notes describe a "new example application," which is correct. However, the guidelines state:

> Release notes are NOT required for:
> - Test-only changes (unit tests, functional tests)

This is an **example application** that validates PMDs, not a test in `app/test/`. The release notes entry is appropriate. **No issue here.**

---

### 2. Documentation could clarify that this is NOT a unit test
**File:** `doc/guides/sample_app_ug/wycheproof_validation.rst`

The documentation is clear that this is an example application. The usage instructions show how to run it as a standalone tool. **No issue here.**

---

### 3. Global `env` structure should be documented
**File:** `examples/wycheproof_validation/main.c:73`

The global `struct app_env env` holds all application state. While this is acceptable for an example, a comment explaining its purpose would improve readability. This is a **minor style suggestion**, not a warning.

---

### 4. `parse_uint32` could use `strtoul` errno handling more defensively
**File:** `examples/wycheproof_validation/main.c:106`

The function checks `errno != 0` after `strtoul`, but does not explicitly set `errno = 0` before the call. While the current code is correct (the caller doesn't set errno, and strtoul will set it on error), defensive coding would clear errno first:

```c
errno = 0;
parsed = strtoul(text, &end, 10);
```

**Severity:** This is a **minor robustness suggestion**, not a warning. The current code is acceptable.

---

### 5. Missing explicit comparison in dequeue loop (line 494)
**File:** `examples/wycheproof_validation/main.c:494`

```c
while (rte_cryptodev_dequeue_burst(dev_id, 0, &completed, 1) == 0) {
```

DPDK style prefers explicit comparisons, but this is a direct comparison of a return value to `0`, which is acceptable. **No issue here.**

---

### 6. `compute_hash` uses hash_dev_id but documents "cryptodev auth path"
**File:** `examples/wycheproof_validation/main.c:1174`

The comment says "Compute a plain (keyless) message digest through the cryptodev auth path," but the function uses `env.hash_dev_id`, which may differ from `env.dev_id`. The comment should clarify that it uses the hash device, not the target device.

**Fix:**

```c
/* Compute a plain (keyless) message digest through the hash device's auth path. */
```

---

## INFO (Consider)

### 1. `dequeue_one` timeout is arbitrary
**File:** `examples/wycheproof_validation/main.c:492`

The 1-second timeout (`rte_get_timer_hz()`) is arbitrary. For a stuck PMD, this could delay the tool's failure. Consider making this configurable or documenting why 1 second was chosen.

---

### 2. `MAX_IV_LEN` and `MAX_EC_BYTESIZE` are not derived from PMD capabilities
**File:** `examples/wycheproof_validation/main.c:29-30`

These are hardcoded constants. While they exceed typical use cases, a comment explaining their origin would help.

---

### 3. `AES_CCM_AAD_OFFSET` magic number
**File:** `examples/wycheproof_validation/main.c:32`

The comment explains the offset, but the value `18` is not derived from any header constant. This is acceptable for an example, but fragile.

---

### 4. Return value of `snprintf` checked correctly
**File:** `examples/wycheproof_validation/main.c:2016`

```c
if (snprintf(file_path, sizeof(file_path), "%s/%s", path, entry->d_name) >=
        (int)sizeof(file_path)) {
```

This is correct. **No issue.**

---

## STYLE (Acceptable)

- **Line length:** All lines are under 100 characters.
- **Boolean comparisons:** Explicit comparisons are used where required (e.g., `if (env.debug)` is acceptable as `debug` is `bool`).
- **Function structure:** Return type on own line, opening brace on own line -- correct.
- **No deprecated API usage.**
- **SPDX headers present.**

---

## POSITIVE FINDINGS

- The code correctly uses `rte_malloc` for DMA-accessible buffers (digests, AAD).
- Error paths return distinct error codes (`-ENOMEM`, `-ENOTSUP`, `-ETIMEDOUT`).
- The `dequeue_one` timeout mechanism prevents hangs on stuck PMDs.
- Capability checks are performed before attempting operations.
- The patch adds comprehensive documentation.

---

## CONCLUSION

**Critical issues:** 8 resource leaks on error paths (all related to `session` not being freed on enqueue failures). These must be fixed.

**Fix pattern:** In all functions that allocate a session and have an `out:` label that currently frees the session *after* the label (on success path only), move the `rte_cryptodev_*_session_free` call *inside* the `out:` block so it runs on both success and error paths.

**Example fix (generalized):**

```c
/* Before (wrong): */
out:
    rte_pktmbuf_free(mbuf);
    rte_crypto_op_free(op);
    rte_cryptodev_sym_session_free(env.dev_id, session);  /* Only on success path */
    if (ret != 0) {
        rte_free(*digest);
        *digest = NULL;
    }
    return ret;

/* After (correct): */
out:
    rte_pktmbuf_free(mbuf);
    rte_crypto_op_free(op);
    if (session != NULL)
        rte_cryptodev_sym_session_free(env.dev_id, session);
    if (ret != 0) {
        rte_free(*digest);
        *digest = NULL;
    }
    return ret;
```

Apply this pattern to `run_aead`, `run_hmac`, `run_gmac`, `compute_hash`, `run_dsa_verify`, `run_ecdh_ecpoint`, and `run_ecdsa_verify`.