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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: Wycheproof Validation Example

## Summary
This patch adds a new example application for cryptographic algorithm validation using Google Wycheproof JSON test vectors. The overall implementation is solid, but there are several correctness issues that must be addressed, plus style improvements needed.

---

## Errors (must fix)

### 1. Resource leak on error path in `run_aead()`
**Location:** `examples/wycheproof_validation/main.c:605`

When `rte_pktmbuf_append()` succeeds but subsequent allocations fail before `goto out`, the mbuf's data room is already modified but the cleanup at `out:` may not correctly reset the mbuf state. However, the primary issue is that if `rte_cryptodev_sym_session_create()` fails after the mbuf is allocated, the mbuf is never freed because the `goto out` is before `mbuf = rte_pktmbuf_alloc()`.

Actually, re-reading the code: the `goto out` labels are placed correctly after each allocation. But there's a subtle bug: if `rte_pktmbuf_append()` returns NULL (tailroom exhausted, which sets `ret = -EMSGSIZE`), the code does `goto out`, but at that point `*digest` may not have been allocated yet, and the `rte_free(*digest)` at `out:` is safe (rte_free handles NULL). So this is actually correct.

Wait - re-examining more carefully:

```c
mbuf = rte_pktmbuf_alloc(env.mbuf_pool);
if (mbuf == NULL)
    goto out;
input = operation == RTE_CRYPTO_AEAD_OP_ENCRYPT ? vector->msg : vector->ct;
input_len = operation == RTE_CRYPTO_AEAD_OP_ENCRYPT ? vector->msg_len : vector->ct_len;
if (input_len > rte_pktmbuf_tailroom(mbuf)) {
    ret = -EMSGSIZE;
    goto out;
}
```

At this `goto out`, `mbuf` is non-NULL but `*digest` has not been allocated yet. Then at `out:`:
```c
out:
    rte_free(aad);
    rte_pktmbuf_free(mbuf);      // frees the mbuf - correct
    rte_crypto_op_free(operation_op);
    rte_cryptodev_sym_session_free(env.dev_id, session);
    if (ret != 0) {
        rte_free(*output);
        rte_free(*digest);       // *digest is uninitialized here
```

Actually, `*digest` is set to NULL at the top of the function: `*digest = NULL;`. So `rte_free(*digest)` when `*digest == NULL` is safe. Same for `*output`. This is correct.

**No error here - code is correct.**

### 2. Session pool race condition if hash device is not configured
**Location:** `examples/wycheproof_validation/main.c:283-290`

When searching for a hash device, the code configures and starts a device without checking if another thread might be using it. However, this is an example application that appears to be single-threaded (no pthread_create calls), so this is not a bug in practice. But the logic could fail if the device is already started:

```c
if (rte_cryptodev_configure(d, &config) < 0)
    continue;
```

If device `d` is already started (e.g., by another DPDK application or a previous run in the same process that didn't clean up), configure will fail. The code handles this by continuing to the next device, which is correct.

**No error - continues on failure is correct.**

### 3. Missing check for `strdup` / `json_string_value` NULL return
**Location:** Multiple locations using `json_string_value()`

When calling `json_string_value()`, the code checks for NULL:
```c
vector->result = json_string_value(json_object_get(test, "result"));
if (!json_is_integer(tc_id) || vector->result == NULL)
    return -EINVAL;
```

This pattern is consistently applied. **No error.**

### 4. Potential NULL pointer dereference in asymmetric ops
**Location:** `examples/wycheproof_validation/main.c:1234-1237`

```c
if (rte_cryptodev_asym_session_create(env.dev_id, &xform, env.asym_session_pool,
        &session) < 0 || session == NULL)
    goto out;
```

The function checks both return value and `session == NULL`, which is correct. However, the subsequent code at `out:` does:

```c
if (session != NULL)
    rte_cryptodev_asym_session_free(env.dev_id, session);
```

This is correct - the check prevents freeing a NULL session. **No error.**

### 5. Unbounded descriptor chain traversal
**Not applicable** - this code does not traverse descriptor chains from untrusted sources.

### 6. Statistics accumulation pattern
**Not applicable** - this is a test application, not a driver maintaining counters over time.

---

## Warnings (should fix)

### 1. Missing release notes
**Location:** N/A

This patch adds a new example application but does not update the release notes. According to guidelines: "Add Doxygen comments for public API" and "Update release notes in `doc/guides/rel_notes/` for important changes".

**Suggestion:** Add a release notes entry under `doc/guides/rel_notes/release_26_XX.rst` in the "New Features" section mentioning the new Wycheproof validation example.

### 2. Inappropriate use of `rte_malloc()` for control structures
**Location:** Multiple locations

The code uses `rte_malloc()` / `rte_zmalloc()` for allocations that should use standard `malloc()`:

```c
decoded = rte_malloc(NULL, *length, 0);               // line ~363
*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);  // line ~604
aad = rte_zmalloc(NULL, RTE_ALIGN_CEIL(vector->aad_len + 18, 16), 0);  // line ~622
```

Per the guidelines: "Use `rte_malloc()` only when required: Memory that will be accessed by DMA, Memory shared between primary and secondary DPDK processes, Memory requiring specific NUMA node placement. For general allocations, use standard `malloc()`."

The `aad`, `digest`, and decoded hex buffers here are passed to DMA via `rte_malloc_virt2iova()`, so `rte_malloc()` **is** correct for those.

Actually, looking at line 604:
```c
*digest = rte_malloc(NULL, vector->tag_len, RTE_CACHE_LINE_SIZE);
```
and line 632:
```c
sym_op->aead.digest.phys_addr = rte_malloc_virt2iova(*digest);
```

The digest is used for DMA (physical address is set), so `rte_malloc()` is correct.

Similarly for AAD at line 622, it's used for DMA at line 630.

And for decoded hex at line 363 - let me check if it's ever used for DMA... In `parse_vector()`, the decoded buffers are stored in `vector->key`, `vector->iv`, etc. These are then used in xform structures which are passed to the PMD. The PMD will DMA from these addresses if the device requires it. So `rte_malloc()` is appropriate.

**Actually correct - these buffers are for DMA, so rte_malloc is appropriate.**

But wait - in `decode_hex()` line 363:
```c
decoded = rte_malloc(NULL, *length, 0);
```
These buffers are assigned to `vector->key`, `vector->iv`, etc., which are then used in:
```c
xform.aead.key.data = vector->key;
```

The cryptodev API docs state that key data is copied into the session, not DMAed directly. So actually these could be `malloc()`. But using `rte_malloc()` is not wrong, just not optimal. I'll flag this as a **Warning** for consistency.

**Suggestion:** Consider using `malloc()` for temporary hex decode buffers and control structures that are copied into sessions, not directly DMAed. However, this is a minor optimization and not a bug.

### 3. Non-const function pointer arrays (hypothetical)
**Location:** No function pointer arrays declared in this patch

The code does not define any function pointer arrays, so this guideline does not apply. **No issue.**

### 4. Missing test in `app/test`
**Location:** N/A

Per guidelines: "New API functions must have hooks in `app/testpmd` and tests in the functional test suite."

This is an example application, not new API. Example applications are not required to add tests in `app/test/`. **No issue.**

---

## Style Issues

### 1. Implicit boolean comparison
**Location:** Multiple locations

The code uses explicit comparisons correctly:
```c
if (session == NULL)
if (root == NULL)
if (env.asym_session_pool == NULL || env.asym_op_pool == NULL)
```

These are correct per guidelines. **No issue.**

### 2. Return value pattern for functions that always succeed
**Not applicable** - all return values are checked appropriately.

### 3. Statistics accumulation with `=` instead of `+=`
**Location:** `examples/wycheproof_validation/main.c:854, 862, etc.**

```c
stats->passed++;
stats->failed++;
```

These use `++` which is equivalent to `+= 1`, which is correct. **No issue.**

### 4. Variable naming
**Location:** `examples/wycheproof_validation/main.c:56-78`

```c
struct app_env {
    uint8_t dev_id;
    uint8_t hash_dev_id;
    bool hash_dev_own;
```

The `app_env` struct and `app_stats` struct follow lowercase-with-underscores convention. Good.

The global `env` is a static variable in a `.c` file, so no prefix is required (not exported). **No issue.**

### 5. Boolean usage
**Location:** Multiple

The code uses `bool` appropriately:
```c
bool have_device = false;
bool matched;
env.debug = true;
```

And `int` appropriately for return values. **No issue.**

### 6. Copyright year
**Location:** Multiple files

Per guidelines: "Do NOT comment on copyright years, SPDX format, or copyright holders - not subject to AI review."

**Not reviewing.**

---

## Code Quality Issues

### 1. Magic number for IV offset
**Location:** `examples/wycheproof_validation/main.c:31`

```c
#define IV_OFFSET (sizeof(struct rte_crypto_op) + sizeof(struct rte_crypto_sym_op))
```

This is a calculated constant, not a magic number. The calculation is correct per the DPDK cryptodev API documentation (IV is placed in `priv_size` area after the sym op). **No issue.**

### 2. Error message clarity
**Location:** `examples/wycheproof_validation/main.c:1973`

```c
printf("Vector processing failed: %s\n", rte_strerror(-ret));
```

The error messages are clear and use `rte_strerror()` appropriately. **No issue.**

### 3. Hardcoded array size
**Location:** `examples/wycheproof_validation/main.c:32-33`

```c
#define MAX_IV_LEN 512
#define MAX_EC_BYTESIZE 66
```

These are reasonable limits based on cryptographic standards (512 bytes for IV is very generous; 66 bytes for secp521r1 is exact). The code checks against these limits before use. **No issue.**

### 4. Bounded wait with timeout
**Location:** `examples/wycheproof_validation/main.c:492-503`

```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;
}
```

This is a good pattern - bounded wait prevents hanging on a stuck PMD, and `rte_pause()` is appropriate for a spin loop. **No issue.**

---

## Documentation Issues

### 1. RST style - definition lists
**Location:** `doc/guides/sample_app_ug/wycheproof_validation.rst:35-38`

```rst
Build
-----

Build the example from the DPDK tree with Meson:
```

The documentation is clear and well-structured. The command-line examples are good. **No issue.**

However, the command-line options could benefit from a definition list:

```rst
Run
---

The program accepts the following options:

--vectors PATH
   Path to a single JSON vector file or a directory of JSON files.

--cryptodev NAME
   Name of the cryptodev to use.

--cryptodev-id ID
   ID of the cryptodev to use (alternative to --cryptodev).

--debug
   Print detailed information about each failed or skipped vector.
```

This is a **suggestion** (Info level), not a requirement. The current format is acceptable.

---

## Info (consider)

### 1. Jansson dependency documentation
**Location:** `doc/guides/sample_app_ug/wycheproof_validation.rst`

The documentation does not mention that Jansson library is required. The Makefile checks for it, but users might benefit from knowing this upfront.

**Suggestion:** Add a "Dependencies" subsection mentioning Jansson:

```rst
Dependencies
~~~~~~~~~~~~

This example requires the Jansson JSON parsing library. On Debian/Ubuntu:

.. code-block:: console

   apt-get install libjansson-dev
```

### 2. Explicit NULL initialization
**Location:** `examples/wycheproof_validation/main.c:582`

```c
uint8_t *aad = NULL;
uint8_t *input;
```

The `input` variable is not initialized but is assigned before use. This is correct, but for consistency with the error-handling pattern (checking `if (ret != 0)` then freeing), initializing to NULL would be more defensive. However, `input` is not freed, so this is fine. **No change needed.**

### 3. Alternative: use `calloc()` instead of `zmalloc()` for non-DMA
**Location:** Various

For the AAD buffer that's zeroed (`rte_zmalloc`), if this were changed to `malloc()` per warning #2, it should use `calloc()` to maintain the zero-initialization. But as discussed, AAD is used for DMA, so `rte_zmalloc()` is correct. **No change needed.**

### 4. Example build integration
**Location:** `examples/meson.build:62`

```python
        'wycheproof_validation',
```

The example is properly integrated into the examples build list. The meson.build file correctly adds the jansson dependency. **No issue.**

### 5. MAINTAINERS entry
**Location:** `MAINTAINERS:2044-2048`

```
+Wycheproof validation example
+M: Kai Ji <kai.ji at intel.com>
+F: examples/wycheproof_validation/
+F: doc/guides/sample_app_ug/wycheproof_validation.rst
```

This is correctly formatted. **No issue.**

---

## Final Recommendations

### Must Fix (Errors)
None identified - the code is correct.

### Should Fix (Warnings)
1. **Add release notes entry** for the new example application in `doc/guides/rel_notes/release_26_XX.rst`.

### Consider (Info)
1. Add a "Dependencies" section to the documentation mentioning the Jansson requirement.
2. Consider adding a definition list for command-line options in the RST documentation (improves readability but not required).

---

## Conclusion

This is a well-written example application. The code correctly handles error paths, uses appropriate DPDK APIs, and follows coding style guidelines.