|WARNING| [v5] examples/rpcapd: remote pcap daemon

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK rpcapd Example Patch Review

## ERRORS

### Resource Leak on Error Path

**capture.c:85-98** - `open_data_listener()`:
```c
fd = socket(addr.ss_family, SOCK_STREAM, 0);
if (fd < 0) {
	RPCAPD_LOG(ERR, "data socket: %s", strerror(errno));
	return -1;
}

if (bind(fd, (struct sockaddr *)&addr, alen) < 0 ||
    listen(fd, 1) < 0 ||
    getsockname(fd, (struct sockaddr *)&addr, &alen) < 0) {
	RPCAPD_LOG(ERR, "data port bind/listen: %s", strerror(errno));
	close(fd);
	return -1;
}
```
The file descriptor `fd` is leaked if any of the three operations in the compound conditional succeeds but a later one fails. The short-circuit evaluation of `||` means that if `bind()` succeeds but `listen()` fails, the `close(fd)` is executed. However, if `listen()` succeeds but `getsockname()` fails, `close(fd)` is also executed, which is correct. The actual issue is that this pattern is correct as written - all failure paths close the fd. Reviewed and verified correct.

### Use-After-Free via Shared Memory After Primary Exit

**capture.c:129-145** - `stop_capture()`:
```c
if (!rte_eal_primary_proc_alive(NULL)) {
	s->capture_on = false;
	s->promisc_set = false;
	s->ring = NULL;
	s->mp = NULL;
	s->prm = NULL;
	if (s->data.fd >= 0) {
		close(s->data.fd);
		s->data.fd = -1;
	}
	return;
}

if (s->capture_on) {
	rte_pdump_disable(s->port, RTE_PDUMP_ALL_QUEUES, s->pdump_flags);
```
After the primary process dies, the code sets pointers to NULL without freeing them. This is documented as intentional ("the shared memory state cannot be trusted"), but creates a resource leak of DPDK shared memory objects (ring, mempool) that survive the secondary process. If `rpcapd` continues running and serves another client after the primary restarts, these leaked shared memory objects remain allocated in hugepages. The comment says "leave the memory alone" but this leaks hugepage memory permanently. A secondary that outlives multiple primary restarts will accumulate these leaks.

The code should either exit when the primary dies, or track whether objects were allocated before or after a primary death so they can be freed safely.

### Missing Error Check

**capture.c:280** - `handle_startcap()`:
```c
data_listen = open_data_listener(c, &data_port);
if (data_listen < 0) {
	stop_capture(s);
	return rpcap_send_error(c, 0, "data port setup failed");
}
```
If `rte_eth_promiscuous_enable()` (line 298) returns an error, the code logs a notice but continues. Subsequently, `rte_pdump_enable_bpf()` (line 307) may fail, but if it succeeds, the capture will be marked as started even though promiscuous mode was requested but not enabled. This is a partial failure that leaves the session in an inconsistent state - the client expects promiscuous mode but didn't get it, and no error is reported.

### Missing Error Check

**main.c:557** - `dpdk_init()`:
```c
for (i = 0; i < RTE_DIM(args); i++) {
	eal_argv[i] = strdup(args[i]);
	if (eal_argv[i] == NULL)
		return -1;
}

if (file_prefix != NULL && *file_prefix != '\0') {
	eal_argv[i++] = strdup("--file-prefix");
	eal_argv[i++] = strdup(file_prefix);
	if (eal_argv[i - 1] == NULL || eal_argv[i - 2] == NULL)
		return -1;
}
```
Memory allocated by successful `strdup()` calls is leaked when a later `strdup()` fails and the function returns -1. The function allocates up to 10 strings but never frees any of them on the error path. All previously allocated strings from `args[]` and potentially `"--file-prefix"` are leaked.

### Double Free Possible

**filter.c:98-106** - `read_filter()`:
```c
rte_free(s->prm);
s->prm = rte_bpf_convert(&bf);
free(insns);
if (s->prm == NULL) {
	RPCAPD_LOG(ERR, "rte_bpf_convert failed: %s",
		rte_strerror(rte_errno));
	return rpcap_send_error(c, 0, "cannot convert filter") < 0 ? -1 : 1;
}
```
If `rte_bpf_convert()` fails and returns NULL, `s->prm` is set to NULL. The caller `handle_updatefilter()` checks the return value and on error path (line 134) calls `rte_free(s->prm)` which was saved in `old` before the call. However, on line 133, if `ret != 0`, the code does `rte_free(s->prm)` where `s->prm` is now NULL (which is safe), then `s->prm = old` to restore the old filter. This is correct. Reviewed - no double free occurs.

---

## WARNINGS

### Missing Release Notes for Internal Implementation

The release notes addition describes this as a user-facing feature:
```
+* **Added libpcap remote capture daemon example.**
+
+  Added the ``dpdk-rpcapd`` example application, which implements the
+  rpcap protocol over loopback to allow live capture in tcpdump and
+  Wireshark.
```
This is appropriate - it's a new example application, not an internal API change. However, the guidelines state that internal API and helper functions do not require release notes. Since this is an example application (not a library change), release notes are appropriate here.

### Statistics Accumulation Pattern Check

**capture.c:412-413**:
```c
npkt = rte_atomic_load_explicit(&s->npkt,
				rte_memory_order_relaxed) + 1;
rte_atomic_store_explicit(&s->npkt, npkt,
			  rte_memory_order_relaxed);
```
This is incrementing a packet counter with load-add-store instead of using `rte_atomic_fetch_add_explicit()`. While functionally correct in this single-writer context (the data thread is the only writer), using the fetch_add atomic would be more idiomatic and clearer about intent. Not an error since there's only one writer, but worth noting as a pattern improvement.

### Non-const Function Pointer in Ops Table?

Searched for function pointer arrays - none found that should be const. The example does not define any ops tables or dispatch arrays.

### Memory Ordering Too Strong?

**capture.c:158**:
```c
rte_atomic_store_explicit(&s->npkt, 0, rte_memory_order_relaxed);
```
This initializes the packet counter when starting capture. Using `relaxed` is correct here - no other data depends on observing this initialization atomically.

**capture.c:586**:
```c
rte_atomic_store_explicit(&s->data_stop, false, rte_memory_order_relaxed);
```
The data thread polls `data_stop` to know when to exit. Using `relaxed` for both the store and load is correct - this is a simple flag and no other memory accesses need to be ordered around it.

All atomic operations in this patch use appropriate memory ordering for their contexts.

---

## INFO

### Code Style Note

**main.c:191-192**:
```c
static void
signal_handler(int sig __rte_unused)
```
The `__rte_unused` annotation is correct, but note that the signal handler only sets an atomic bool. This is async-signal-safe. Good pattern.

### rte_malloc vs malloc Usage

**session.c:23** and elsewhere use `malloc()`/`realloc()` for control-path buffers (interface list, filter programs). This is appropriate per the guidelines - these are not DMA buffers or shared between processes.

**capture.c:111-117** correctly uses `rte_pktmbuf_pool_create_by_ops()` for the capture mempool, which must be in hugepages for DMA and multi-process visibility.

---

## SUMMARY

The patch introduces a remote packet capture daemon example implementing the rpcap protocol for live capture from DPDK applications. The implementation is generally sound with appropriate use of DPDK APIs and atomics.

**Critical issues:**
1. Resource leak when primary process dies and secondary continues - hugepage objects (ring, mempool) are abandoned but not freed, accumulating across multiple primary restarts
2. Missing error handling for `strdup()` failures in `dpdk_init()` - leaks all previously allocated argv strings
3. Inconsistent error handling when promiscuous mode enable fails but capture proceeds

**Other issues:**
- Documentation appropriately describes security limitations and Linux-only support
- Atomic operations use appropriate memory ordering throughout
- Memory allocation patterns follow DPDK guidelines (malloc for control, rte_malloc for shared/DMA)
- Example includes comprehensive documentation in RST format

The errors must be fixed before merging.

Review-Result: ERROR