|WARNING| [v4,3/3] usertools: add script for wireshark capture

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: capture: infrastructure wireshark packet capture

### Errors

**lib/capture/capture.c:224** - Use-after-free risk on error path:
```c
capture_free(cap);

/* return 0 since error reported "error":"XXX" in response */
return 0;
```
After `capture_free(cap)` at line 224, the function returns to `capture_start_req` which then executes `goto error_callback_remove` at line 417, reaching line 419 where `capture_remove_callbacks(cap)` is called. The `cap` pointer has already been freed at line 224.

**lib/capture/output.c:278** - Potential memory leak:
```c
capture_unlink(cap);
if (orphaned) {
    /* ... */
    CAPTURE_LOG(ERR,
                "capture %u: secondary still attached, leaking ring and pool",
                cap->idx);
} else {
    capture_free(cap);
}

return 0;
```
When `orphaned` is true, `cap` itself is not freed (only the ring and pool are intentionally leaked per the comment), but the `cap` structure allocated at line 244 of capture.c is never reclaimed.

**lib/capture/datapath.c:107** - NULL pointer dereference:
```c
p = rte_pcapng_copy(port_id, queue_id, pkts[i], mp, snaplen, direction, NULL);
if (unlikely(p == NULL))
    stats->nombuf++;
else
    dup_bufs[d_pkts++] = p;
```
If `prog` is NULL (capture has no filter) and `rte_pcapng_copy` returns NULL, `stats->nombuf++` is executed. However, when `prog` is not NULL and the filter rejects a packet, we execute `stats->filtered++` and `continue` (lines 100-102), never reaching the pcapng_copy. The issue is that when the allocation fails, the accepted counter has not been incremented yet (it happens at line 115), so the accepted+filtered+nombuf totals will not account for this packet properly. This is not a NULL dereference but a logic error in counter accounting.

**lib/capture/multiproc.c:87** - Resource leak on error path:
```c
if (capture_attach_queues(cap, sec->cbs, &sec->nb_cbs, &sec->prog) < 0) {
    capture_detach_queues(cap, sec->cbs, &sec->nb_cbs, sec->prog);
    free(sec);
    return -ENOBUFS;
}
```
`capture_attach_queues` can fail partway through, having allocated `prog` and attached some queues. `capture_detach_queues` is called to clean up, but `sec->nb_cbs` may not reflect all attached queues if the failure happened mid-loop, and the `prog` pointer in `sec` may be uninitialized. The code at line 86 initializes `sec` with `calloc`, so `sec->nb_cbs` starts at 0 and `sec->prog` is NULL, but `capture_attach_queues` writes into `*progp` (line 329 of datapath.c) before any queue is attached, so if that succeeds but a later queue fails, `sec->prog` will be set but `sec->nb_cbs` will be the count of successfully attached queues only. The detach then unloads `sec->prog` with the count from `sec->nb_cbs`, which is correct. However, if `capture_attach_queues` fails at line 307 (filter load failure), it returns -1 without setting `*progp`, so `sec->prog` remains NULL and the detach call at line 87 passes NULL for `prog`, which is safe (filter_unload checks for NULL at line 151). The lea
 k risk is if the filter is compiled (shared memory from `rte_malloc` at line 89 of filter.c) but the load fails in a secondary - the compiled filter (`cap->filter`) is never freed by the secondary. But the secondary only receives `cap` as a shared pointer and never owns `cap->filter`; only the primary frees it in `capture_free` at line 218 of capture.c. So this is not a leak after all.

**lib/capture/filter.c:95** - `rte_free(prm)` on an object allocated by pcap:
```c
prm = rte_bpf_convert(&bf);
pcap_freecode(&bf); /* drop the cBPF program */

if (prm == NULL)  {
    CAPTURE_LOG(ERR, "BPF convert interface %s(%d)",
                rte_strerror(rte_errno), rte_errno);
    goto error;
}
```
At line 100, `rte_free(prm)` is called in the error path. However, `prm` comes from `rte_bpf_convert`, which is a DPDK function that returns an `rte_bpf_prm` struct. Looking at the DPDK docs, `rte_bpf_convert` allocates the `prm->ins` array but the caller is responsible for freeing it and the `prm` wrapper. The code at line 102 uses `rte_free(prm)`, which is correct if `rte_bpf_convert` used `rte_malloc` for `prm`. Checking the reference, `rte_bpf_convert` does use `rte_malloc` for both `prm` and `prm->ins`, so `rte_free(prm)` is correct. Not an error.

**lib/capture/capture.c:395** - Error path at line 395 (`error_callback_remove`) does not call `capture_mp_broadcast(cap, CAPTURE_MP_STOP)`:
```c
error_unlink:
    /* the drain thread never ran, so undo the attach here */
    if (capture_mp_broadcast(cap, CAPTURE_MP_STOP) < 0) {
        CAPTURE_LOG(ERR,
                    "capture %u: secondary still attached, leaking ring and pool",
                    cap->idx);
        capture_unlink(cap);
        capture_remove_callbacks(cap);
        return 0;
    }
    capture_unlink(cap);
error_callback_remove:
    capture_remove_callbacks(cap);
    capture_free(cap);
```
At `error_callback_remove` (line 418), if the thread-create failed (goto from line 414), `capture_mp_broadcast(cap, CAPTURE_MP_STOP)` was already done at line 399, so skipping it here is intentional. But if we arrived via the first `goto error_callback_remove` at line 368 (callback registration failed), no broadcast was done and secondaries are still attached. The code does not detach them before freeing. This is a leak: the secondaries hold references to `cap->ring` and `cap->mp` which are freed here, corrupting the secondary's datapath.

**lib/capture/capture.c:368** - Missing IPC cleanup before `goto error_callback_remove`:
```c
if (capture_add_callbacks(cap) < 0) {
    capture_err(d, "can not register callbacks");
    goto error_callback_remove;
}
```
The secondaries were told to attach at line 377, but if that succeeds and `capture_add_callbacks` fails at line 366, the path to `error_callback_remove` at line 418 does not broadcast a STOP. Secondaries remain attached to a `cap` that is about to be freed.

### Warnings

**lib/capture/datapath.c:244** - Missing Doxygen for internal function:
```c
static struct capture_rxtx_cb *
capture_attach(struct capture *cap, uint16_t queue, bool is_rx,
               struct capture_filter_prog *prog)
```
This is an internal static function, so Doxygen is not required. The exported functions (`capture_attach_queues`, `capture_detach_queues`, etc.) are in the private header and are internal API, not public, so they also do not require public Doxygen. No issue.

**lib/capture/output.c:57** - `asprintf` return value not checked:
```c
if (uname(&uts) == 0 && asprintf(&osname, "%s %s", uts.sysname, uts.release) < 0)
    osname = NULL;
```
This checks for `< 0` which is correct for `asprintf` failure. Not an issue.

**lib/capture/capture.c:183** - Hardcoded snaplen default duplicates definition:
```c
cfg->snaplen = snaplen > 0 ? snaplen : UINT32_MAX;
```
The comment at line 179 says "Default is 256K from tcpdump legacy", and `DEFAULT_SNAPLEN` is defined as `262144u` at line 48, but the default is applied in `parse_params` at line 183 only if the user passes `snaplen=0`. The `struct capture_config` is initialized with `snaplen = DEFAULT_SNAPLEN` via the designated initializer? No, the initializer at line 119 is `uint32_t snaplen = DEFAULT_SNAPLEN;` which is a local variable, not the struct field. The struct is on the stack and uninitialized except for the fields explicitly set. So if the user does not pass `snaplen=`, the `cfg->snaplen` field is uninitialized. But the caller in `capture_start_req` initializes the struct with `struct capture_config cfg = { .queue = ALL_QUEUES };`, which zero-initializes all other fields including `snaplen`. So `cfg.snaplen` starts at 0, and line 183 sets it to `UINT32_MAX` if the user passed `snaplen=0`, or to the user's value otherwise. But if the user did not pass `snaplen=` at all, it stays 0 and is
  passed to `capture_alloc`, where it is used at line 269 to decide the mbuf size. A snaplen of 0 would make `mbuf_size = rte_pcapng_mbuf_size(0)`, which might be wrong. But the loop at line 152 only sets `snaplen` if the key is `"snaplen"`, so if the user did not pass it, `cfg.snaplen` remains 0. This is a bug: the default is not applied when `snaplen=` is omitted, only when `snaplen=0` is explicitly passed.

**lib/capture/capture.c:119** - Shadowing of `snaplen` variable:
```c
static int
parse_params(char *str, struct capture_config *cfg, struct rte_tel_data *d)
{
    uint32_t snaplen = DEFAULT_SNAPLEN;
```
This local `snaplen` is used to accumulate the parsed value, then assigned to `cfg->snaplen` at line 183. However, the assignment only happens inside the `snaplen=0` branch, and the default value is never written to `cfg` when the parameter is omitted. This is a logic error, not shadowing.

**lib/capture/meson.build:19** - Warning message uses deprecated phrasing:
```c
else
    warning('libpcap is missing, capture filtering will be disabled')
endif
```
This is a build warning, which is acceptable. The wording is clear.

**lib/capture/capture.c:48** - `DEFAULT_SNAPLEN` value does not match comment:
```c
#define DEFAULT_SNAPLEN		262144u	/* from tcpdump legacy */
```
The comment says "from tcpdump legacy", and 262144 is indeed the tcpdump default as of recent versions. Consistent.

**lib/capture/capture.c:322** - Return value of `rte_thread_create` not fully checked:
```c
int ret = rte_thread_create(&thread_id, NULL, capture_thread, cap);
if (ret != 0) {
    capture_err(d, "thread start failed: %s", strerror(ret));
    goto error_unlink;
}
```
`rte_thread_create` returns 0 on success or a positive error code on failure. The code checks `!= 0` and uses `strerror(ret)`, which is correct.

**lib/capture/datapath.c:115** - Statistics counter increment without atomic:
```c
stats->accepted += d_pkts;
```
The `stats` pointer is to `cbs->stats`, which is a per-callback block. The callback runs on the datapath, and the control path reads the stats under `cap->cbs_lock`. This is a data race: the datapath writes `stats->accepted` without holding the lock. However, the guidelines state that "control plane versus datapath races are findings" but "data races between ethdev control-path callbacks" are not (the application serializes those). This is a datapath callback versus control-path read race, so it should be flagged. But the counter is `uint64_t`, and on most architectures a 64-bit write is atomic if aligned. The struct is `__rte_cache_aligned`, so the `accepted` field at offset 0 is 64-bit aligned. On x86_64 and aarch64, a naturally aligned 64-bit store is atomic. However, the increment `+=` is a read-modify-write, which is not atomic even if the individual read and write are. Another datapath thread could be incrementing the same counter concurrently (if two lcores call the same Rx/Tx
  callback on different queues? No, each callback block is per-queue, and the datapath does not share queue state across lcores in the normal case). But the callback is registered per queue, so only one lcore should be inside a given `cbs` at a time. The control path reads the counter under `cbs_lock`, but the datapath writes without holding it. This is a race, but the lock is a spinlock, and the datapath cannot take it (that would block the fast path). The solution would be to use `rte_atomic_fetch_add_explicit(&stats->accepted, d_pkts, rte_memory_order_relaxed)`, but the guidelines say "Statistics accumulation using `=` instead of `+=`" is an error (overwrite vs increment). This is a `+=`, so it's correct in that sense, but the lack of atomicity is a separate issue. However, the guidelines also say "Statistics use `+=` not `=` for accumulation" (line 635 of AGENTS.md), which implies `+=` is the expected pattern, not that it must be atomic. The atomic requirement is separate: "Share
 d variables use `rte_atomic_*_explicit()`, not `volatile` or bare access" (line 626). So this is a shared variable (read by control path, written by datapath) using a bare `+=` instead of an atomic. This is an error.

**lib/capture/datapath.c:90** - `RTE_ASSERT` in fast path:
```c
RTE_ASSERT(nb_pkts <= CAPTURE_BURST_SIZE);
```
This is a debug check, and `RTE_ASSERT` is compiled out in production builds (unless `RTE_ENABLE_ASSERT` is set). It's acceptable for a sanity check on internal logic.

**lib/capture/capture.c:390** - Potential resource leak if thread detach fails:
```c
rte_thread_detach(thread_id);
```
`rte_thread_detach` on POSIX is `pthread_detach`, which can fail (e.g., if the thread already exited). If it fails, the thread is not detached, and when it exits its resources are not reclaimed until this process exits or until `pthread_join` is called on it (which we never do). However, the return value is not checked. This is a potential resource leak, but it's unlikely in practice because the thread was just created. Still, the return value should be checked.

**lib/capture/capture.c:54** - Log level set to NOTICE:
```c
RTE_LOG_REGISTER_DEFAULT(rte_capture_logtype, NOTICE);
```
`NOTICE` is a reasonable default for a library: it's less verbose than `INFO` but still reports important events. The user can adjust it at runtime. Not an issue.

### Summary of Errors

1. **capture.c:224 / capture.c:419** - Use-after-free: `capture_free(cap)` at line 224 inside `capture_alloc` error path, then `capture_remove_callbacks(cap)` at line 419 after returning to `capture_start_req`.
2. **capture.c:368** - Missing `capture_mp_broadcast(cap, CAPTURE_MP_STOP)` before `goto error_callback_remove` when callback registration fails but secondaries were already toldwarning: review has no Review-Result line, severity inferred from prose (response may be truncated)