|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 Patch Review

## ERRORS

### capture.c

**Line 113: Resource leak on error path**
```c
return rte_pktmbuf_pool_create_by_ops(name, ring_size * 2, 0, 0,
				      RTE_MBUF_DEFAULT_BUF_SIZE,
				      rte_socket_id(), "ring_mp_mc");
```
The `create_capture_ring()` function allocates a ring on line 96 (`rte_ring_create()`), but if `create_capture_mempool()` fails later in `handle_startcap()` (line 236), the ring is freed but the mempool allocation failure path does not clean up the ring created earlier. Actually, reviewing further: on line 239-242, `stop_capture()` is called which does free the ring. This is correct.

**Line 232-242: Ring leaked if mempool creation fails before assignment**
```c
s->ring = create_capture_ring(s->port);
s->mp = create_capture_mempool(s->port);
if (s->ring == NULL || s->mp == NULL) {
	RPCAPD_LOG(ERR, "ring/mempool alloc failed: %s",
		rte_strerror(rte_errno));
	stop_capture(s);
	return rpcap_send_error(c, 0, "DPDK alloc failed");
}
```
If `create_capture_ring()` succeeds but `create_capture_mempool()` fails, `s->ring` is assigned the non-NULL ring pointer, so `stop_capture()` will correctly free it. However, if `create_capture_ring()` itself returns NULL but allocates internal state before failing, that state is leaked. Checking the implementation: `rte_ring_create()` cleans up on failure, so this is safe.

**Line 259-265: Data listener fd leaked on pdump enable failure**
```c
if (rte_pdump_enable_bpf(s->port, RTE_PDUMP_ALL_QUEUES, s->pdump_flags,
			 0, s->ring, s->mp, s->prm) < 0) {
	RPCAPD_LOG(ERR, "rte_pdump_enable_bpf port %u failed: %s",
		s->port, rte_strerror(rte_errno));
	close(data_listen);
	stop_capture(s);
	return rpcap_send_error(c, 0, "cannot enable capture");
}
```
The `data_listen` fd is closed on this error path (line 262), which is correct.

**Line 290: Data listener fd leaked on send failure**
```c
if (rpcap_send_msg(c, RPCAP_MSG_STARTCAP_REPLY, 0, &reply, sizeof(reply)) < 0) {
	close(data_listen);
	stop_capture(s);
	return -1;
}
```
The listener is closed here as well. This is correct.

**Line 299: Data listener fd closed but data_fd leak on accept failure**
```c
data_fd = accept_from(data_listen, &s->peer, DATA_ACCEPT_TIMEOUT_MS);
close(data_listen);
if (data_fd < 0) {
	stop_capture(s);
	return -1;
}
```
`accept_from()` returns -1 on failure, so `data_fd < 0` is true and no fd is leaked. This is correct.

### filter.c

**Line 68-70: `insns` leaked on error path**
```c
insns = calloc(nitems, sizeof(*insns));
if (insns == NULL) {
	if (rpcap_discard(c, plen) < 0)
		return -1;
	return rpcap_send_error(c, 0, "out of memory") < 0 ? -1 : 1;
}
```
If `rpcap_discard()` fails (returns -1), the function returns -1 immediately without freeing `insns`. This is a memory leak.

**Suggested fix:**
```c
insns = calloc(nitems, sizeof(*insns));
if (insns == NULL) {
	rpcap_discard(c, plen);
	return rpcap_send_error(c, 0, "out of memory") < 0 ? -1 : 1;
}
```
Or:
```c
if (insns == NULL) {
	if (rpcap_discard(c, plen) < 0) {
		/* still return the error to client if possible */
		rpcap_send_error(c, 0, "out of memory");
		return -1;
	}
	return rpcap_send_error(c, 0, "out of memory") < 0 ? -1 : 1;
}
```

**Line 142-157: Old filter program not freed if pdump re-enable fails**
```c
if (rte_pdump_enable_bpf(s->port, RTE_PDUMP_ALL_QUEUES, s->pdump_flags,
			 0, s->ring, s->mp, s->prm) < 0) {
	RPCAPD_LOG(ERR, "rte_pdump_enable_bpf port %u failed: %s",
		s->port, rte_strerror(rte_errno));
	rte_free(old);
	/* The capture cannot be resumed */
	stop_capture(s);
	return rpcap_send_error(c, 0, "cannot apply filter");
}
s->capture_on = true;

/* Safe now that the old program is no longer referenced. */
rte_free(old);
```
The old filter is freed on both the success path (line 157) and the failure path (line 149). On the failure path, `stop_capture()` is called after freeing `old`, and `stop_capture()` will call `rte_free(s->prm)` on line 182 of capture.c. At this point `s->prm` points to the new filter (assigned on line 128 of filter.c via `read_filter()`), so the new filter is freed. The old filter was already freed on line 149. Both are freed once each, which is correct. No double-free occurs.

### main.c

**Line 560-566: EAL argv array elements leaked**
```c
eal_argv = calloc(eal_argc + 1, sizeof(char *));
if (eal_argv == NULL)
	return -1;

for (i = 0; i < RTE_DIM(args); i++) {
	eal_argv[i] = strdup(args[i]);
	if (eal_argv[i] == NULL)
		return -1;
```
If `strdup()` fails, the function returns -1 without freeing the previously allocated strings in `eal_argv[0]` through `eal_argv[i-1]`, nor the `eal_argv` array itself.

**Suggested fix:**
```c
for (i = 0; i < RTE_DIM(args); i++) {
	eal_argv[i] = strdup(args[i]);
	if (eal_argv[i] == NULL)
		goto cleanup;
}
...
cleanup:
	for (unsigned int j = 0; j < i; j++)
		free(eal_argv[j]);
	free(eal_argv);
	return -1;
```

**Line 570-575: Same leak for file_prefix and lcore_arg strdup failures**
```c
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;
}
```
Same pattern: if either `strdup()` fails, previously allocated strings are leaked.

### session.c

**Line 20-41: Memory leak in handle_findallif on realloc failure or send error**
```c
RTE_ETH_FOREACH_DEV(p) {
	...
	nb = realloc(buf, buflen + entry);
	if (nb == NULL) {
		RPCAPD_LOG(ERR, "out of memory in findallif");
		free(buf);
		return rpcap_send_error(c, 0, "out of memory");
	}
	buf = nb;
	...
}
...
rc = rpcap_send_msg(c, RPCAP_MSG_FINDALLIF_REPLY, nif, buf, buflen);
free(buf);
return rc;
```
On line 32, if `realloc()` fails, `buf` is freed and an error is sent. This is correct. On line 63, `buf` is freed after sending. If `rpcap_send_msg()` fails and returns -1, `buf` is still freed on line 64, so no leak. This is correct.

---

## WARNINGS

### capture.c

**Line 520-529: Data thread exit signal handling could race**
```c
static void
stop_data_thread(struct session *s)
{
	if (!s->data_running)
		return;

	rte_atomic_store_explicit(&s->data_stop, true, rte_memory_order_relaxed);
	rte_thread_join(s->data_thread, NULL);
	s->data_running = false;

	close(s->data_done_fd);
	s->data_done_fd = -1;
}
```
The `s->data_done_fd` is closed immediately after joining the thread. If another thread were to call `wait_ctrl_or_data_exit()` concurrently (which is not the case in this single-client design, but worth noting), there could be a race. However, this is a single-client daemon and `stop_data_thread()` is only called from the control thread after the data thread has exited, so this is safe in practice.

### filter.c

**Line 127-134: Error handling discards old filter on malformed request**
```c
struct rte_bpf_prm *old = s->prm;
int ret;

s->prm = NULL;
ret = read_filter(c, plen, s);
if (ret != 0) {
	/* Malformed request: keep running with the old filter. */
	rte_free(s->prm);
	s->prm = old;
	return ret < 0 ? -1 : 0;	/* error already reported */
}
```
If `read_filter()` allocates a new `s->prm` before failing, line 133 frees it and restores the old filter. This is correct. However, if `read_filter()` fails before allocating anything, `s->prm` is NULL and `rte_free(NULL)` is called, which is safe (rte_free handles NULL). The old filter is restored. This is correct, not a warning.

### main.c

**Line 607-610: CPU affinity restore failure logged but not fatal**
```c
if (lcore_arg == NULL &&
    rte_thread_set_affinity_by_id(rte_thread_self(), &cpuset) != 0)
	RPCAPD_LOG(INFO, "Can not restore original CPU affinity");
```
Failure to restore affinity is logged at INFO level and the daemon continues. This is acceptable for a daemon that may run on any CPU, but operators should be aware the affinity may not be as expected.

### meson.build (examples/rpcapd/meson.build)

**Line 21-27: Sources list uses files() which is correct**
```python
sources = files(
        'capture.c',
        'filter.c',
        'main.c',
        'session.c',
        'sock.c',
)
```
This is the correct meson pattern.

---

## INFO

### capture.c

**Line 26: BURST_SIZE and other constants could be configurable**
```c
#define BURST_SIZE                    32
#define POLL_INTERVAL_MS              500
```
These are hardcoded. Making them configurable (e.g., via command-line options) could improve flexibility, but is not required for an example application.

**Line 145: Comment about primary process death is helpful**
```c
/*
 * If the primary is gone the shared memory state cannot be trusted:
 * pdump IPC has no one to answer it, the port belongs to a dead
 * process, and the ring, mempool and heap locks may have been held
 * when it died.  Drop the references and leave the memory alone.
 */
```
This is good documentation of a subtle correctness concern.

### Documentation (rpcapd.rst)

**Line 54: Security section is clear and appropriate**
The documentation clearly states this is an example, not a production tool, and explains the security limitations. This is good practice.

### Overall code quality

The patch is well-structured, with clear separation between connection handling (sock.c), session management (session.c), filtering (filter.c), and capture (capture.c). Error handling is generally thorough, with most paths checking return values and cleaning up on failure.

---

Review-Result: ERROR