|WARNING| [v3] examples/rpcapd: demo version of packet capture daemon

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: examples/rpcapd

## Summary

This patch adds a new example application implementing a remote packet capture daemon (rpcapd) using DPDK's pdump framework. The implementation is a secondary process that allows libpcap-based tools (tcpdump, Wireshark) to capture live traffic from DPDK ports over the rpcap protocol.

---

## Errors

### 1. Use-after-free in `handle_updatefilter`

**Location:** `examples/rpcapd/main.c:777-782`

```c
/* Malformed request: keep running with the old filter. */
rte_free(s->prm);
s->prm = old;
return ret < 0 ? -1 : 0;	/* error already reported */
```

**Issue:** When `read_filter()` returns an error (`ret != 0`), the code frees the newly allocated `s->prm` and restores the old pointer. However, this is only correct when `read_filter()` successfully allocated a new filter before failing validation. If `read_filter()` returns 1 (validation error) with `s->prm` still NULL (allocation failed inside `read_filter`), the code attempts to free NULL and then assigns the old filter back. While freeing NULL is safe, there's a logic error: if `read_filter()` fails early (e.g., OOM in `calloc()`), `s->prm` never gets set to the new allocation, so freeing it is wrong.

**Correct pattern:**

```c
struct rte_bpf_prm *old = s->prm;
int ret;

s->prm = NULL;
ret = read_filter(fd, plen, s);
if (ret != 0) {
	/* read_filter may have allocated s->prm before failing;
	 * free it if so, then restore the old filter.
	 */
	rte_free(s->prm);
	s->prm = old;
	return ret < 0 ? -1 : 0;
}

/* New filter succeeded; now safe to free the old one. */
if (!s->capture_on) {
	rte_free(old);
	return rpcap_send_msg(fd, RPCAP_MSG_UPDATEFILTER_REPLY, 0, NULL, 0);
}
/* ... re-enable capture with new filter ... */
rte_free(old);
```

The current code is actually **correct** as written: `read_filter()` sets `s->prm` to the result of `rte_bpf_convert()` only on success; on failure paths (OOM, bad length, invalid program), it either never allocates or frees the intermediate `insns` before returning, leaving `s->prm` unchanged (NULL from the `s->prm = NULL` assignment at line 773). The `rte_free(s->prm)` at line 778 is therefore freeing a NULL pointer (safe), and the restore of `old` is correct.

**Retraction:** This is **not** an error. The code is correct; `read_filter` only sets `s->prm` on full success.

---

### 2. Resource leak on `read_filter` error after pdump is armed

**Location:** `examples/rpcapd/main.c:786-803`

```c
if (rte_pdump_enable_bpf(s->port, RTE_PDUMP_ALL_QUEUES, s->pdump_flags,
			 s->snaplen, 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, so do not leave the ring,
	 * mempool and data connection behind: the client has been
	 * told the capture is over, and a session that is neither
	 * capturing nor torn down has no way back.
	 */
	stop_capture(s);
	return rpcap_send_error(fd, 0, "cannot apply filter");
}
s->capture_on = true;

/* Safe now that the old program is no longer referenced. */
rte_free(old);
```

**Issue:** If `rte_pdump_enable_bpf()` fails (line 786), the code frees `old` (line 790), calls `stop_capture(s)` (line 797), then returns an error to the client. However, `stop_capture()` will attempt to free `s->prm` (the new filter) at line 660 (`rte_free(s->prm)`), which is correct. But `old` was already freed at line 790, and if `old == s->prm` somehow (it shouldn't be, since `s->prm` is the new filter), there could be a double-free. 

Actually, re-reading the code: `old` is the **previous** filter saved at line 772. `s->prm` is the **new** filter from `read_filter()`. So freeing both is correct: `old` at line 790, then `s->prm` inside `stop_capture()` at line 660. This is **not** a double-free.

However, there's a different issue: the error path at lines 788-798 frees `old` (line 790) before calling `stop_capture()`, but `stop_capture()` also frees `s->prm` (the new filter). The two are distinct allocations, so this is correct.

**Retraction:** This is **not** an error. The code correctly frees the old filter explicitly, then lets `stop_capture()` free the new filter.

---

### 3. Missing cleanup of `insns` on error after `bpf_validate` fails

**Location:** `examples/rpcapd/main.c:742-746`

```c
/* Reject a malformed program here */
if (!bpf_validate(bf.bf_insns, bf.bf_len)) {
	free(insns);
	return rpcap_send_error(fd, 0, "invalid filter program") < 0 ? -1 : 1;
}
```

**Issue:** If `rpcap_send_error()` fails (returns < 0), the function returns -1, which propagates up to `handle_client()` and terminates the session. This is correct. On the other path (send succeeds, return 1), the function returns 1 to signal "validation error, already reported". The `insns` array was freed at line 743, so this is correct.

**Retraction:** This is **not** an error. The code correctly frees `insns` before returning.

---

## Warnings

### 1. `handle_updatefilter` re-enables capture without checking for ring drain

**Location:** `examples/rpcapd/main.c:786-803`

The code disables the pdump callback, then immediately re-enables it with the new filter. The comment at line 771 states "Packets already in the ring are kept; only the brief gap between disable and enable is lost." However, if the ring has a large backlog, packets captured with the **old** filter will be sent to the client **after** the UPDATEFILTER_REPLY, potentially confusing clients that expect the new filter to take effect immediately.

**Suggested improvement:** Consider documenting this behavior in the RST file's Limitations section, or drain the ring before re-enabling capture (though this would lose those packets).

---

### 2. Statistics counter `s->npkt` wraps at UINT32_MAX without notice

**Location:** `examples/rpcapd/main.c:963`

```c
s->npkt++;
```

The `npkt` field is `uint32_t` (line 83) and wraps silently after 4 billion packets. The rpcap protocol's `rpcap_pkthdr.npkt` is also 32-bit, so this is inherent to the protocol. However, a long-running capture could wrap and confuse clients.

**Recommendation:** Document this limitation in the RST file if not already noted.

---

### 3. IPv6 support claimed but not tested in documentation

**Location:** `doc/guides/sample_app_ug/rpcapd.rst`

The `-b` option accepts numeric IPv4 or IPv6 addresses (line 61 of the RST), but there's no IPv6 example in the document, and the warning about exposing traffic to the network mentions only the default IPv4 loopback.

**Suggested improvement:** Add an IPv6 example or explicitly state IPv6 is supported but not demonstrated.

---

### 4. `capture_loop` exit condition may not be promptly detected

**Location:** `examples/rpcapd/main.c:1003-1063`

The main loop checks `quit_signal` only once per iteration of `capture_loop()`, and if the ring is constantly draining (high traffic), the check at line 1013 may not run for an extended period. A SIGTERM could be delayed by seconds.

**Mitigation already present:** The `check_socket_status()` call at line 1016 uses `poll(..., 0)` which returns immediately, and the outer `while` condition at line 1013 checks `quit_signal`, so this is bounded by the burst-processing time. Not a serious issue, but worth noting.

---

### 5. Hardcoded `DEFAULT_SNAPLEN` may not match client expectations

**Location:** `examples/rpcapd/main.c:64, 822-824`

```c
#define DEFAULT_SNAPLEN   RTE_MBUF_DEFAULT_DATAROOM
...
s->snaplen = ntohl(req.snaplen);
if (s->snaplen == 0 || s->snaplen > DEFAULT_SNAPLEN)
	s->snaplen = DEFAULT_SNAPLEN;
```

`RTE_MBUF_DEFAULT_DATAROOM` is 2048 bytes, but standard Ethernet MTU is 1514 (or up to 9000 for jumbo frames). A client that requests snaplen=0 (capture whole packet) will be silently clamped to 2048, which may truncate jumbo frames. This is documented in the Limitations section (line 208 of RST), so it's acceptable, but consider whether the clamp should be higher (e.g., 16384) to handle jumbo frames.

---

### 6. `dpdk_init` leaks `eal_argv` strings on allocation failure

**Location:** `examples/rpcapd/main.c:1284-1334`

```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;  /* leaks previously strdup'd strings */
}
```

If any `strdup()` fails in the loop, the function returns -1 without freeing the already-allocated strings. Since this is a fatal error (the program exits via `rte_exit` at line 1381), the leak is harmless in practice, but it's sloppy.

**Suggested fix:** Add a cleanup loop on error, or accept that a malloc failure during init is a terminal condition where leaks don't matter.

---

### 7. `send_iov_full` does not handle `MSG_MORE` flag correctly on EINTR

**Location:** `examples/rpcapd/main.c:273-299`

The function retries on `EINTR` (line 281), but it passes `flags | MSG_NOSIGNAL` to `sendmsg()` and expects the `MSG_MORE` flag to survive the retry. If `sendmsg()` is interrupted after sending some bytes, the retry resumes with `MSG_MORE` still set, which is correct. However, if the caller passes `MSG_MORE` on the **last** call of a burst but that call is interrupted and retried, `MSG_MORE` is reapplied when it should have been cleared. This could delay the final segment being sent.

**Severity:** Low; `MSG_MORE` is a hint, not a requirement, and the final segment will be flushed when the next call without `MSG_MORE` is made (or when the connection is closed).

---

## Info

### 1. Consider using `bool` for boolean flags

**Locations:** Several fields in `struct session`

```c
struct session {
	...
	bool     opened;			/* OPEN_REQ has selected a port */
	bool     capture_on;
	bool     promisc_set;			/* we enabled promiscuous mode */
	...
};
```

**Good:** The code already uses `bool` for these fields. No change needed.

---

### 2. `MAX_FILTER_INSNS` is arbitrary

**Location:** `examples/rpcapd/main.c:70`

```c
#define MAX_FILTER_INSNS   4096
```

This bound prevents a malicious client from sending a huge filter that exhausts memory, which is good. However, 4096 BPF instructions is quite large (typical filters are <100 instructions). Consider documenting why this limit was chosen, or reducing it to a more realistic value like 1024.

---

### 3. Documentation uses RST definition lists correctly

**Location:** `doc/guides/sample_app_ug/rpcapd.rst` lines 54-104

The command-line options are formatted as a definition list, which is the preferred RST style per the guidelines. No change needed.

---

### 4. Release notes entry is appropriate

**Location:** `doc/guides/rel_notes/release_26_11.rst`

The patch adds a release notes entry for the new example, which is correct per the guidelines (release notes are required for new drivers/subsystems/examples). The entry is concise and in the correct section.

---

### 5. Functional tests not included

**Location:** N/A

The patch adds a new example application but does not include functional tests (no changes under `app/test/`). The guidelines state that new API functions must have tests, but this is an **example application**, not a library API. Examples are not required to have unit tests.

**Recommendation:** Consider adding a basic test that starts the daemon, connects a mock client, and verifies the findallif response. However, this is **not** required for an example.

---

## Summary of Findings

**Errors:** 0 (all initial concerns were retracted upon closer inspection)

**Warnings:** 7
- UPDATEFILTER ring handling may confuse clients
- Packet counter wraps at UINT32_MAX
- IPv6 support not demonstrated in docs
- Quit signal detection latency under high traffic
- Snaplen clamping may truncate jumbo frames
- `dpdk_init` leaks strings on allocation failure (harmless)
- `send_iov_full` may delay final segment on EINTR with MSG_MORE

**Info:** 5 observations (all positive or minor suggestions)

---

## Overall Assessment

This is a **well-written example** with careful attention to resource management, error handling, and documentation. The code correctly uses atomics for the quit signal, cleans up resources on all paths, and handles the complexities of the rpcap protocol in a constrained subset suitable for an example.

The warnings are **minor**: the packet counter wrap and snaplen clamp are inherent to the protocol or documented limitations; the resource leak on init failure is harmless since it's a fatal error; the MSG_MORE behavior is a performance detail, not correctness.

**Recommendation:** Accept the patch with the warnings as documentation improvements or minor code hygiene, not blockers.