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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-13

# Wycheproof Validation Example - Code Review

## ERRORS

**Error: Missing error checks on function return values**

Line 218 (app_init):
```c
session_size = rte_cryptodev_sym_get_private_session_size(env.dev_id);
{
    /* The hashing device may differ from the target device, so size the
     * session pool for the largest sym session across all devices.
     */
    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;
    }
}
```

`rte_cryptodev_sym_get_private_session_size()` can return 0 for devices that don't support symmetric crypto. Using an unchecked 0 as a size parameter to pool creation may cause initialization failures or allocation issues.

**Error: Resource leak on error path**

Line 202-226 (app_init):
```c
ret = rte_cryptodev_configure(env.dev_id, &config);
if (ret < 0)
    return ret;

env.mbuf_pool = rte_pktmbuf_pool_create("WYCHEPROOF_MBUF_POOL", 64, 0, 0,
    env.mbuf_data_room, rte_socket_id());
if (env.mbuf_pool == NULL) {
    ret = -rte_errno;
    goto error;
}
```

After `rte_cryptodev_configure()` succeeds but before any pool allocations, if an error occurs the cryptodev remains configured but is never stopped or closed. The `error:` label does not call `rte_cryptodev_close()` until after all pools are freed, but the device was configured before the pools existed. An early failure (e.g., mbuf_pool creation) leaves the device in a configured state without cleanup.

Suggested fix: The `error:` label should call `rte_cryptodev_close(env.dev_id);` to clean up the configured device.

**Error: Hash device not stopped/closed on main device init failure**

Line 285-292 (app_init error path):
```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);
    ...
    rte_cryptodev_close(env.dev_id);
    return ret;
```

If the hash device was successfully started (`env.hash_dev_own = true` on line 273) but a later initialization step fails (e.g., `rte_cryptodev_start(env.dev_id)` on line 247 fails), the error path does not stop or close `env.hash_dev_id`. This leaks the hash device state.

Suggested fix: Add cleanup for the hash device before closing the main device in the error path.

**Error: Unchecked return value from rte_cryptodev_start**

Line 247 (app_init):
```c
ret = rte_cryptodev_start(env.dev_id);
if (ret < 0)
    goto error;
```

Line 268-277 (hash device initialization):
```c
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);
    continue;
}
```

For the hash device fallback path, if `rte_cryptodev_start(d)` fails after `rte_cryptodev_queue_pair_setup()` succeeds, only `rte_cryptodev_close(d)` is called. However, `rte_cryptodev_close()` requires the device to be stopped first. Calling close on a started-but-failed device is incorrect.

Suggested fix: If start fails, explicitly stop the device before closing it, or check whether stop is needed based on the start failure.

**Error: Integer overflow in hex_value calculation**

Line 354-357 (decode_hex):
```c
for (index = 0; index < *length; index++) {
    int high = hex_value(hex[index * 2]);
    int low = hex_value(hex[index * 2 + 1]);
```

When `index` approaches `SIZE_MAX / 2`, the expression `index * 2` can overflow on 32-bit systems (where `size_t` is 32 bits). However, this is guarded by the earlier check `hex_length / 2 > UINT32_MAX` on line 344, which limits `*length` to at most `UINT32_MAX`. On 64-bit systems this is not an issue. On 32-bit systems, the guard prevents the loop from reaching overflow. This is borderline but acceptable given the guard.

Actually, reviewing more carefully: `hex_length` is `strlen()` result (size_t), and `hex_length / 2` is compared to `UINT32_MAX`. If `hex_length` is `2 * UINT32_MAX + 1`, then `hex_length / 2 = UINT32_MAX`, which passes the check, and `*length = UINT32_MAX`. Then in the loop, when `index = UINT32_MAX / 2`, `index * 2` is still within `UINT32_MAX` on 32-bit. When `index` approaches `UINT32_MAX`, `index * 2` would overflow 32 bits, but `index` never exceeds `*length - 1 = UINT32_MAX - 1`, so `index * 2 <= 2 * (UINT32_MAX - 1)`, which does overflow on 32-bit size_t.

Wait, the loop condition is `index < *length`, so `index` ranges from 0 to `*length - 1`. If `*length = UINT32_MAX`, then `index` can be `UINT32_MAX - 1`, and `index * 2 = 2 * (UINT32_MAX - 1)`, which on a 32-bit system (where size_t is uint32_t) overflows.

This is a potential overflow on 32-bit platforms when the hex string length is exactly `2 * UINT32_MAX` bytes.

Suggested fix: Either limit `*length` to `UINT32_MAX / 2`, or cast index to size_t before multiplication: `(size_t)index * 2`.

**Error: Resource leak in run_aead when op enqueue fails**

Line 556-558 (run_aead):
```c
if (rte_cryptodev_enqueue_burst(env.dev_id, 0, &operation_op, 1) != 1) {
    ret = -EIO;
    goto out;
}
```

If `enqueue_burst` fails, the `operation_op` crypto op is still allocated and is not freed before `goto out`. The `out:` label does call `rte_crypto_op_free(operation_op)`, so this is actually correct. However, the comment at line 561-563 states that if `dequeue_one` returns NULL (device wedged), the op is still owned by the PMD and should not be freed. But here, if enqueue fails (returns 0), the op was never accepted by the PMD and should be freed. The current code does free it via the `out:` label, so this is correct.

Upon closer inspection, this is not a leak. The `out:` label frees `operation_op`, and that is reached when enqueue fails. This is correct.

**Error: Potential use-after-free in dequeue_one timeout path**

Line 470-478 (dequeue_one):
```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;
}
```

Line 561-564 (run_aead):
```c
completed = dequeue_one(env.dev_id);
if (completed == NULL)
    /* Device wedged: op still owned by the PMD. Abort without freeing
     * in-flight state; teardown stops the device.
     */
    return -ETIMEDOUT;
```

When `dequeue_one` times out and returns NULL, the comment states the op is still owned by the PMD and should not be freed. The function returns -ETIMEDOUT without calling the cleanup at `out:`, which is correct. However, `operation_op`, `mbuf`, `session`, `aad`, and `*digest` are all still allocated. The caller (`validate_aead_vector`) receives -ETIMEDOUT and propagates it up to `process_aead`, which then returns -ETIMEDOUT to `process_file`, which prints an error and exits. The `app_uninit()` is called in `main()`, which stops the device, but the leaked allocations (mbuf, session, digest buffer, aad) are never freed.

This is a resource leak on the timeout path. The device is stopped in `app_uninit()`, so the PMD no longer owns the op, but the application never frees the mbuf, session, or digest buffers that were allocated before the timeout.

Suggested fix: On -ETIMEDOUT return, add a comment that leaks are intentional because the device is wedged, or track in-flight ops and free them after stopping the device in app_uninit. Alternatively, clarify in the doc that a device timeout is a fatal error and cleanup is not guaranteed.

**Error: Missing session_size validation before pool creation**

Line 218-230 (app_init):
```c
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;
    }
}
env.session_pool = rte_cryptodev_sym_session_pool_create("WYCHEPROOF_SESSION_POOL",
    64, session_size, 0, 0, rte_socket_id());
```

If all devices return 0 from `rte_cryptodev_sym_get_private_session_size()`, `session_size` remains 0, and the session pool is created with element size 0, which may fail or produce a non-functional pool. The code does not check for `session_size == 0` before pool creation.

Suggested fix: After the loop, check `if (session_size == 0) { ret = -ENOTSUP; goto error; }`.

---

## WARNINGS

**Warning: Missing release notes enforcement for new public API**

The patch does not add any new public DPDK API (no new rte_* functions in installed headers), so release notes for API are not required. However, the MAINTAINERS entry and the release notes update are present, which is appropriate for a new example application. This is correct.

**Warning: Misleading comment about device closure timing**

Line 268-277 (app_init, hash device fallback):
```c
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);
    continue;
}
```

If `rte_cryptodev_queue_pair_setup()` succeeds but `rte_cryptodev_start()` fails, the device is in a configured-but-not-started state. The code calls `rte_cryptodev_close()` without first calling `rte_cryptodev_stop()`. According to DPDK API, a device should be stopped before being closed. However, if the device was never started (start failed), calling stop may not be necessary. The DPDK cryptodev API does not explicitly require stop before close if start never succeeded, but it's safer to always stop before close if configure succeeded.

Suggested fix: Always call `rte_cryptodev_stop(d)` before `rte_cryptodev_close(d)` in this error path, even if start failed.

**Warning: No validation of mbuf_data_room against actual device limits**

Line 159-163 (parse_args):
```c
case 'm':
    if (parse_uint32(optarg, &env.mbuf_data_room) != 0 ||
            env.mbuf_data_room < RTE_PKTMBUF_HEADROOM ||
            env.mbuf_data_room > UINT16_MAX)
        return -EINVAL;
```

The `--mbuf-dataroom` option is validated only against `RTE_PKTMBUF_HEADROOM` and `UINT16_MAX`, but the selected cryptodev may have a maximum segment size or other constraints that are violated by this user-supplied value. The code does not query `dev_info` to check `max_nb_segments` or segment size limits before creating the mbuf pool.

Suggested fix: After device selection, validate `env.mbuf_data_room` against `dev_info.max_nb_segments` and reject values that would require segmentation the device cannot handle.

**Warning: Loop counter variable name collision risk**

Multiple functions (process_aead, process_hmac, process_gmac, process_dsa_p1363, process_ecdh_ecpoint, process_ecdsa_p1363) all use `group_index` and `test_index` as loop counters. While these are in different scopes and do not actually collide, the naming pattern is repetitive and slightly increases the risk of copy-paste errors. This is a minor style concern, not a functional issue.

No action needed, but consider using more distinct names if extending the code.

---

## INFO

**Info: Hardcoded pool sizes may be too small for large test suites**

Line 208-210, 233-235, 240-242 (app_init):
```c
env.mbuf_pool = rte_pktmbuf_pool_create("WYCHEPROOF_MBUF_POOL", 64, 0, 0,
    env.mbuf_data_room, rte_socket_id());
...
env.session_pool = rte_cryptodev_sym_session_pool_create("WYCHEPROOF_SESSION_POOL",
    64, session_size, 0, 0, rte_socket_id());
...
env.op_pool = rte_crypto_op_pool_create("WYCHEPROOF_OP_POOL",
    RTE_CRYPTO_OP_TYPE_SYMMETRIC, 64, 0, MAX_IV_LEN, rte_socket_id());
```

All pools are sized at 64 objects. The application processes one vector at a time, so this is sufficient for the current design. However, if the application were extended to pipeline multiple operations concurrently, these pool sizes would need to be increased. The current single-threaded, synchronous design makes 64 a reasonable choice.

**Info: Pre-existing code - documentation could clarify acceptable result policy**

The documentation states:
> unsupported vectors, capability-skipped cases, and acceptable results are reported in the summary without forcing a failure.

The code skips all "acceptable" results (lines 617-620, 720-722, etc.). The Google Wycheproof "acceptable" classification means "weak but not technically invalid" (e.g., short HMAC tags, malleable signatures). The decision to skip these rather than validate them is a policy choicewarning: review has no Review-Result line, severity inferred from prose (response may be truncated)