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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - examples/wycheproof_validation

## Summary

This patch adds a new example application that validates cryptodev PMDs against Google Wycheproof test vectors. The code is generally well-structured, but several correctness bugs were identified that must be fixed before merging.

---

## ERRORS (Must Fix)

### 1. Resource leak on error path in `run_aead()`

**File:** `examples/wycheproof_validation/main.c:561-563`

The `aad` buffer allocated for AES-CCM is not freed on the `ret = -EMSGSIZE` error path when input length exceeds mbuf tailroom.

```c
if (input_len > rte_pktmbuf_tailroom(mbuf)) {
    ret = -EMSGSIZE;
    goto out;  /* aad may be allocated but is not freed */
}
```

The `aad` allocation happens after this check in the current code, but the check is positioned incorrectly -- it should come before any resource allocation. The logic should be:

```c
/* Check size limits before any allocation */
if (input_len > rte_pktmbuf_tailroom(mbuf)) {
    ret = -EMSGSIZE;
    goto out;
}

mbuf = rte_pktmbuf_alloc(env.mbuf_pool);
/* ... rest of allocations ... */
```

Alternatively, ensure `aad` is initialized to NULL at function entry and always freed in the cleanup path.

---

### 2. Unchecked `snprintf()` truncation treated as hard error

**File:** `examples/wycheproof_validation/main.c:2024-2028`

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

This aborts directory traversal on the first file with a long path, leaving all remaining files unprocessed. A single over-length filename should not be fatal. The correct pattern is to skip that file and continue:

```c
if (snprintf(file_path, sizeof(file_path), "%s/%s", path, entry->d_name) >=
        (int)sizeof(file_path)) {
    if (env.debug)
        printf("SKIP %s/%s: path too long\n", path, entry->d_name);
    stats->skipped_unsupported++;
    continue;  /* not break */
}
```

---

### 3. `directory` not closed on `snprintf()` error path

**File:** `examples/wycheproof_validation/main.c:2024-2028`

When `snprintf()` fails (line 2026), the function breaks out of the loop but does not close `directory` before returning. The `closedir()` call is after the loop, so it will be reached, but if a future change reorders the logic, this becomes a leak. However, the current code structure does reach `closedir()`, so this is **not** an active bug -- just a fragile pattern.

**Correction:** On re-examination, the code does close the directory (line 2033). This is not an error. ~~Omit this item.~~

---

### 4. `stat()` error handling loses errno detail

**File:** `examples/wycheproof_validation/main.c:2003`

```c
if (stat(path, &path_stat) != 0)
    return -errno;
```

This is correct. ~~Omit this item.~~

---

### 5. Boolean comparison against literal true/false

**File:** Multiple locations

The code uses explicit `== true` and `== false` comparisons on `bool` variables:

- Line 90: `if (env.debug)` (correct, but mixed with explicit comparisons elsewhere)

This is a style inconsistency, not an error. Per the guidelines, boolean variables should use direct truthiness (`if (var)` not `if (var == true)`). However, the code is already mostly correct (e.g., line 90). Only flag if there are explicit `== true` comparisons, which I do not see in this patch.

**Correction:** The code does not contain `== true` or `== false` comparisons. ~~Omit this item.~~

---

### 6. Potential NULL dereference in `run_dsa_verify()` and `run_ecdh_ecpoint()`

**File:** `examples/wycheproof_validation/main.c:1221, 1445`

After `rte_cryptodev_asym_session_create()` returns success (rc >= 0) but `session == NULL`, the code treats it as `-ENOTSUP` and jumps to `out`, where it calls `rte_cryptodev_asym_session_free(env.dev_id, session)` with `session == NULL`.

```c
if (rte_cryptodev_asym_session_create(..., &session) < 0 || session == NULL) {
    ret = -ENOTSUP;
    goto out;
}
/* ... */
out:
    /* ... */
    if (session != NULL)
        rte_cryptodev_asym_session_free(env.dev_id, session);
```

The cleanup path checks `if (session != NULL)` before freeing, so this is safe. ~~Omit this item.~~

---

### 7. `opendir()` failure returns `-errno` but does not set meaningful value

**File:** `examples/wycheproof_validation/main.c:2008-2009`

```c
directory = opendir(path);
if (directory == NULL)
    return -errno;
```

This is correct. `opendir()` sets `errno` on failure per POSIX. ~~Omit this item.~~

---

### 8. Asymmetric session not freed on enqueue failure

**File:** `examples/wycheproof_validation/main.c:1237-1240` (DSA verify)

```c
if (rte_cryptodev_enqueue_burst(env.dev_id, 0, &op, 1) != 1) {
    ret = -EIO;
    goto out;
}
```

The `out:` label includes `rte_cryptodev_asym_session_free()`, so the session is freed. This is correct. ~~Omit this item.~~

---

### 9. `rte_zmalloc()` return not checked before passing to `rte_cryptodev_asym_session_create()`

**File:** `examples/wycheproof_validation/main.c:1437-1439` (ECDH)

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

This is correct -- the check happens immediately after allocation. ~~Omit this item.~~

---

### 10. Initialization of `env` with designated initializer does not zero all fields

**File:** `examples/wycheproof_validation/main.c:71-73`

```c
static struct app_env env = {
    .mbuf_data_room = DEFAULT_MBUF_DATA_ROOM,
};
```

In C, designated initializers zero-initialize all unspecified fields. This is correct. ~~Omit this item.~~

---

### ACTUAL ERROR: Race condition in PMD op completion check

**File:** `examples/wycheproof_validation/main.c:507-516`

```c
static struct rte_crypto_op *
dequeue_one(uint8_t dev_id)
{
    struct rte_crypto_op *completed = NULL;
    uint64_t deadline = rte_get_timer_cycles() + rte_get_timer_hz();

    while (rte_cryptodev_dequeue_burst(dev_id, 0, &completed, 1) == 0) {
        if (rte_get_timer_cycles() > deadline)
            return NULL;
        rte_pause();
    }
    return completed;
}
```

**Issue:** `completed` is a local variable (on the stack), not shared between threads. There is no race condition here -- this is single-threaded polling. ~~Omit this item.~~

---

### ACTUAL ERROR: Missing bounds check on `group_index` and `test_index` JSON iteration

**File:** Multiple `json_array_foreach()` loops

The `json_array_foreach()` macro is provided by Jansson and handles bounds checking internally. This is not a bug. ~~Omit this item.~~

---

## Re-Review for Correctness Bugs

After careful re-examination, **only one genuine error remains:**

### ERROR: `ret = -EMSGSIZE` path may skip cleanup in `run_aead()`

**Location:** `examples/wycheproof_validation/main.c:561-563`

If `input_len > rte_pktmbuf_tailroom(mbuf)` after `mbuf` is allocated but before other resources, the function jumps to `out:` which will attempt to free `aad` (if AES-CCM path was taken). However, examining the code flow:

1. Line 536: `operation_op = rte_crypto_op_alloc(...)`
2. Line 540: `mbuf = rte_pktmbuf_alloc(...)`
3. Line 543: Size check and potential `-EMSGSIZE`
4. Line 575: `aad = rte_zmalloc(...)` (only for CCM, inside an `if` block starting line 574)

So the order is: `operation_op` - `mbuf` - size check - `aad` (conditionally).

The size check happens **before** `aad` allocation. Therefore, there is no leak of `aad` on the `-EMSGSIZE` path.

**Correction:** This is not a bug. ~~Omit this item.~~

---

## FINAL ERROR SECTION

After full analysis, **no correctness bugs** meeting the >=50% confidence threshold were found. The error paths are correctly structured, resources are cleaned up, and the NULL checks are in place.

---

## WARNINGS (Should Fix)

### 1. Use of `rte_malloc()` for general-purpose allocations

**Locations:** Throughout (e.g., `decode_hex()`, digest buffers, operand buffers)

`rte_malloc()` allocates from hugepage memory, which is a limited resource. Per the guidelines, general-purpose allocations should use standard `malloc()`. Only memory accessed by DMA or shared between processes requires hugepage backing.

**Examples to change:**
- Line 393: `decoded = rte_malloc(...)` in `decode_hex()` -- change to `malloc()`
- Line 558: `*digest = rte_malloc(...)` in `run_aead()` -- change to `malloc()` (digest is copied from/to DMA but not directly used by DMA)
- Line 667: `*digest = rte_malloc(...)` in `run_hmac()` -- same
- Line 801: `*digest = rte_malloc(...)` in `run_gmac()` -- same
- Line 1171: `op_digest = rte_malloc(...)` in `compute_hash()` -- this one is used in `op->sym->auth.digest.data`, so it **is** accessed by DMA -- keep `rte_malloc()`
- Line 1436: `out_x = rte_zmalloc(...)` in `run_ecdh_ecpoint()` -- used in `op->asym->ecdh.shared_secret.x.data`, accessed by DMA -- keep

**Recommendation:**
Change all `rte_malloc()` / `rte_zmalloc()` calls to `malloc()` / `calloc()` **except**:
- Buffers assigned to `op->sym->...` or `op->asym->...` (DMA descriptors)
- The AAD buffer in `run_aead()` (line 575) if it's passed to the PMD (it is: `sym_op->aead.aad.data`)

Specifically, the allocations in `decode_hex()` are for temporary parsing of JSON hex strings -- these should be `malloc()`.

---

### 2. Missing release notes entry for new public API (N/A)

This example does not add public API. No release notes required beyond what is already present. ~~Omit.~~

---

### 3. Overly broad skip on `snprintf()` failure

Already covered in ERRORS section (though reclassified as correctness issue). See ERROR #2.

---

### 4. No verification that `env.hash_dev_id` is actually started

**Location:** `examples/wycheproof_validation/main.c:281-307`

The code searches for a symmetric hash-capable device and attempts to configure/start it. If `rte_cryptodev_start()` fails, the code closes the device and continues the search. However, after the loop, the code does not verify that `env.hash_dev_own == true` implies the hash device is actually started.

Looking closer: if the `start()` fails, the code closes the device and does not set `env.hash_dev_own = true` (lines 299-303). So if no suitable device is found, `env.hash_dev_own` remains false and `env.hash_dev_id == env.dev_id`. This is correct.

~~Omit this item.~~

---

### 5. `env.session_pool` sizing loop uses local variable `s` shadowing function scope

**Location:** `examples/wycheproof_validation/main.c:209-217`

```c
uint32_t session_size;
/* ... */
session_size = rte_cryptodev_sym_get_private_session_size(env.dev_id);
{
    uint8_t d;
    for (d = 0; d < rte_cryptodev_count(); d++) {
        uint32_t s = rte_cryptodev_sym_get_private_session_size(d);
        if (s > session_size)
            session_size = s;
    }
}
```

The variable `s` is in a nested block, not shadowing anything. This is fine. ~~Omit.~~

---

### 6. Potential integer overflow in `sig_len / 2` for DSA/ECDSA

**Location:** `examples/wycheproof_validation/main.c:1376, 1791`

`sig_len` is `uint32_t`. Division by 2 cannot overflow. ~~Omit.~~

---

## INFO (Consider)

### 1. `dequeue_one()` timeout is hardcoded to 1 second

This may be too short for slow hardware or high system load. Consider making it configurable or documenting the assumption.

---

### 2. Error messages in `compute_hash()` and asymmetric paths do not identify which device failed

When debugging PMD issues, it would be helpful to include the device ID in error/timeout messages.

---

### 3. `MAX_EC_BYTESIZE` is 66 but not validated against Jansson-decoded operand lengths

The code checks `if (bytesize > sizeof(priv_buf))` but only after attempting to normalize. Consider an early bounds check on the decoded lengths from JSON.

---

### 4. `normalize_operand()` silently strips leading zeros

This is intentional (big-endian unsigned normalization), but a comment explaining why would help maintainability.

---

### 5. Example does not demonstrate testpmd integration

New API examples typically include testpmd hooks. This is an example application, not a library API, so testpmd integration is not required. ~~Omit.~~

---

## CODE STYLE

All style issues below are **informational** -- the code is readable and follows DPDK conventions.

### 1. Booleans use direct truthiness (correct)

The code correctly uses `if (env.debug)` rather than `if (env.debug == true)`. No issues.

---

### 2. Lines are within 100 characters

Verified -- no lines exceed 100 characters.

---

### 3. `rte_` prefix used correctly

External symbols and example name follow DPDK conventions.

---

## SUMMARY

**Total Findings:**
- **Errors:** 1 (snprintf error path aborts instead of skipping)
- **Warnings:** 1 (use of rte_malloc for non-DMA allocations)
- **Info:** 4 (timeout hardcoded, error messages, bounds