|WARNING| [v2] 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/169845

_AI Code Review_

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

# DPDK Patch Review: examples/rpcapd

## Summary

This patch adds a new example application `rpcapd` implementing the remote packet capture (rpcap) protocol for DPDK. The implementation is generally well-structured and follows most DPDK conventions. However, there are several correctness bugs and style issues that must be addressed.

---

## Errors (must fix)

### Correctness Bugs

1. **Resource leak on getaddrinfo() path** (main.c:157)
   ```c
   if (rc != 0)
       rte_exit(EXIT_FAILURE, "Invalid bind address '%s': %s\n",
                str, gai_strerror(rc));
   ```
   `rte_exit()` does not return, so `freeaddrinfo(res)` at line 159 is unreachable. The `res` pointer allocated by `getaddrinfo()` must be freed on the error path.
   
   **Fix:**
   ```c
   rc = getaddrinfo(str, NULL, &hints, &res);
   if (rc != 0)
       rte_exit(EXIT_FAILURE, "Invalid bind address '%s': %s\n",
                str, gai_strerror(rc));
   memcpy(&listen_addr, res->ai_addr, res->ai_addrlen);
   listen_addrlen = res->ai_addrlen;
   freeaddrinfo(res);
   ```
   Actually, this is correct -- the early exit is intentional. **Retract this item.**

2. **Potential use-after-free in handle_updatefilter()** (main.c:842-869)
   The function saves `s->prm` to `old`, sets `s->prm = NULL`, calls `read_filter()` which may set `s->prm` to a new program. If `read_filter()` fails, the code restores `s->prm = old` and frees `s->prm` (line 848-849). However, on success when `s->capture_on` is false (line 853-857), the function frees `old` and returns. But if `s->capture_on` is true and the `rte_pdump_enable_bpf()` call fails (line 862-870), the code calls `stop_capture(&s)` which frees `s->prm` at line 669, then returns an error without freeing `old`. The `old` pointer is leaked.
   
   **Fix:** Add `rte_free(old);` before the error return at line 869:
   ```c
   if (rte_pdump_enable_bpf(...) < 0) {
       RPCAPD_LOG(ERR, ...);
       rte_free(old);  /* Free the old program before cleanup */
       stop_capture(s);
       return rpcap_send_error(fd, 0, "cannot apply filter");
   }
   ```

3. **`rte_thread_get_affinity_by_id()` return value not checked** (main.c:1282)
   ```c
   if (lcore_arg == NULL &&
       rte_thread_get_affinity_by_id(rte_thread_self(), &cpuset) != 0)
       rte_panic("rte_thread_getaffinity failed\n");
   ```
   This function can fail, and the code correctly checks it. However, if it succeeds, `cpuset` is used at line 1296. But if `lcore_arg != NULL`, the check is skipped and `cpuset` remains uninitialized (zero-initialized at line 1259). This is correct for the `lcore_arg != NULL` case because the affinity is not restored. **This is not an error.**

4. **Missing bounds check on client-supplied `plen` before allocation** (main.c:367-406)
   In `handle_findallif()`, the code reallocates `buf` to `buflen + entry` where `entry` includes `namelen` and `desclen` from the device, not from the client. The `plen` parameter is not used in this function -- it comes from the message header but FINDALLIF_REQ carries no payload (the payload length should be zero). The function does not read any client data, so there is no attack surface here. **Not an error.**

5. **Unbounded loop in `recv_full()` if peer never sends** (main.c:240-258)
   The `while (len > 0)` loop relies on `wait_readable()` to enforce a timeout, but `wait_readable()` at line 249 is called with `timeout_ms = -1`, meaning wait indefinitely. If the client never sends, the loop blocks forever. However, `wait_readable()` polls in `POLL_INTERVAL_MS` slices and checks `quit_signal` on each iteration (line 177), so a signal or dead primary will eventually break the loop. This is by design. **Not an error.**

6. **Potential integer overflow in `handle_findallif()` buffer size** (main.c:386)
   ```c
   nb = realloc(buf, buflen + entry);
   ```
   If many ports are present, `buflen + entry` could overflow `size_t`. However, DPDK typically has a small number of ports (< 100), and each entry is on the order of 100 bytes, so the total is well under 4GB. Unlikely to be a practical issue, but worth bounding. **Warning rather than Error.**

7. **Array `eal_argv` allocation failure not checked** (main.c:1247-1248)
   ```c
   eal_argv = calloc(eal_argc + 1, sizeof(char *));
   if (eal_argv == NULL)
       return -1;
   ```
   The function returns `-1` on failure, but the caller at line 1344 does not check the return value:
   ```c
   if (dpdk_init() < 0)
       rte_exit(EXIT_FAILURE, "EAL init failure\n");
   ```
   Actually, the caller *does* check `< 0`, so the error is propagated. **Not an error.**

8. **Array `eal_argv[i]` strdup() failures not fully handled** (main.c:1251-1274)
   The code checks `if (eal_argv[i] == NULL) return -1;` after each `strdup()`, but does not free the previously allocated strings in `eal_argv[0..i-1]`. This leaks memory on error. However, the error path calls `rte_exit()` which terminates the process, so kernel cleanup reclaims the leak. Still, this is sloppy.
   
   **Fix:** Either document that the error path intentionally relies on process exit cleanup, or add cleanup code. Given that `rte_exit()` is called immediately after, this is **Warning** rather than Error.

9. **Missing error check on `setsockopt()` in `open_listen_socket()`** (main.c:1185)
   ```c
   setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one));
   ```
   The return value is not checked. On Linux, `setsockopt(SO_REUSEADDR)` on a TCP socket with valid arguments always succeeds. Failure would be from a kernel bug or out-of-memory condition severe enough that the subsequent bind/listen will also fail. Not checking the return is acceptable. **Not an error.**

10. **Shared variable `quit_signal` accessed without atomic load in some paths** 
    Actually, all accesses use `rte_atomic_load_explicit()` or `rte_atomic_store_explicit()`. **Not an error.**

11. **Potential sign extension in timestamp conversion** (main.c:947-948)
    ```c
    .timestamp_sec = htonl((uint32_t)tv.tv_sec),
    .timestamp_usec = htonl((uint32_t)tv.tv_usec),
    ```
    `tv_sec` and `tv_usec` are `time_t` and `suseconds_t` (both signed). Casting to `uint32_t` truncates, but on 64-bit systems with large timestamps (year 2038+), the sign bit of the lower 32 bits may be set. However, the cast to `uint32_t` makes this unsigned truncation, not sign extension. `htonl()` then operates on an unsigned value. **Not an error.**

### Actual Errors Found

After careful review, the only correctness bug is:

**Error 1: Resource leak of `old` filter program in `handle_updatefilter()` error path** (main.c:862-870)

When `rte_pdump_enable_bpf()` fails while `s->capture_on` is true, the code calls `stop_capture(s)` which frees `s->prm` (the new program), but does not free `old` (the previous program saved at line 844). The old program is leaked.

**Fix:**
```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);    /* Must free the saved program before cleanup */
    stop_capture(s);
    return rpcap_send_error(fd, 0, "cannot apply filter");
}
```

---

## Warnings (should fix)

1. **Incomplete cleanup on `dpdk_init()` error paths** (main.c:1251-1274)
   The loop allocating `eal_argv[i]` with `strdup()` returns `-1` on allocation failure but does not free the previously allocated strings. While the process exits immediately after via `rte_exit()`, leaving the cleanup to the kernel, this is sloppy and violates the principle of explicit resource management.
   
   **Suggestion:** Add a cleanup label or document the intentional reliance on process-exit cleanup.

2. **Hardcoded string in `handle_findallif()` description** (main.c:376)
   ```c
   static const char desc[] = "DPDK port";
   ```
   This is a user-visible string but does not identify which DPDK port. Consider including the port number or name in the description for clarity.

3. **Potential `buflen + entry` overflow in `handle_findallif()`** (main.c:386)
   While unlikely in practice (DPDK has few ports, each entry is small), the sum could theoretically overflow `size_t` if an adversary could somehow create thousands of ports. Consider adding an upper bound check.

4. **Missing Doxygen comments on new API functions**
   Wait, this is an example application, not a library. There is no public API. **Retract.**

5. **Release notes do not mention limitations**
   The release notes (doc/guides/rel_notes/release_26_11.rst) say "Added an example that implements rpcap to allow live capture in tcpdump." The documentation file (rpcapd.rst) has a detailed Limitations section, but the release notes do not mention that this is single-client, TCP-only, etc. For a sample application this is acceptable. **Info rather than Warning.**

6. **`MAX_FILTER_INSNS` value not justified** (main.c:70)
   ```c
   #define MAX_FILTER_INSNS 4096
   ```
   This bounds the client-supplied filter size. The value is reasonable (cBPF programs are rarely that large), but a comment explaining the limit would help.

7. **`handle_findallif()` could return partial list on allocation failure** (main.c:388-394)
   If `realloc()` fails, the function frees `buf` and returns an error to the client. However, if some interfaces were already added to `buf` before the failure, the client gets an error and no list, rather than a partial list. This is correct behavior (all-or-nothing), but could be documented.

---

## Info (consider)

1. **`recv_full()` and `send_iov_full()` could be shared utilities**
   These are common socket I/O patterns that might benefit from extraction to a utility file if other examples need them. For a single example, inline is fine.

2. **`wait_readable()` timeout parameter could use a named constant**
   The `-1` for "wait indefinitely" is conventional but a `#define WAIT_INDEFINITELY -1` would self-document.

3. **`process_ring()` burst size is small**
   ```c
   #define BURST_SIZE 32
   ```
   Under high packet rates, a larger burst might reduce overhead. However, 32 is a common DPDK default and works well. Consider making it tunable if performance becomes an issue.

4. **`handle_client()` logs "client disconnected" even on error**
   The function always logs "client %s disconnected" at line 1162, even if the client crashed or was killed. "client %s session ended" might be more neutral.

5. **Example does not demonstrate best practices for multi-client support**
   The documentation notes that multi-client support requires an event-driven main loop (planned). For an example, single-client is fine, but a comment in the code pointing to where the architecture would change could help future contributors.

6. **No tests for the example**
   Examples typically do not have unit tests in `app/test`, but a functional test or at least a README with a worked example would help users. The documentation includes an Example section, which is sufficient.

---

## C Coding Style

1. **Implicit comparison on boolean** (main.c:454, 658, etc.)
   ```c
   if (s->opened) {
   ```
   DPDK style requires explicit comparison: `if (s->opened != false)` for `bool` or `if (s->opened)` is acceptable for actual `bool` types. The `opened` field is declared as `bool` at line 88, so direct truthiness is correct. **Not a violation.**

2. **Implicit comparison on pointer** (main.c:563, 629, 650, etc.)
   ```c
   if (s->ring == NULL) {
   ```
   Explicit comparison to `NULL` is used. **Correct.**

3. **Implicit comparison on integer** (main.c:245, etc.)
   ```c
   while (len > 0) {
   ```
   Explicit comparison. **Correct.**

4. **Function `parse_bind_addr()` return type is `void` but could fail** (main.c:142)
   The function calls `rte_exit()` on error, which does not return. However, if it were to be refactored to return an error code, the signature would need to change. Current implementation is acceptable for an example.

5. **`likely()` not used on hot-path conditionals**
   The `wait_readable()` loop (line 177) checks `quit_signal` on every iteration. This could use `if (unlikely(rte_atomic_load_explicit(...)))` for a micro-optimization. For an example, plain conditionals are fine.

6. **Variable `rc` declared far from use** (main.c:149)
   ```c
   int rc;
   rc = getaddrinfo(str, NULL, &hints, &res);
   ```
   C99 allows declaration at point of use: `int rc = getaddrinfo(...)`. Current style is acceptable but slightly dated.

7. **Magic number `0x100` in enum** (main.c:1210)
   ```c
   enum {
       OPT_LONG_ONLY = 0x100,
   ```
   This is a standard getopt_long() pattern for long-only options. No issue.

8. **Inconsistent use of `unsigned int` vs `uint32_t`** (main.c:885, 1010, etc.)
   The code uses `unsigned int` for loop counters and `uint32_t` for protocol fields. This is correct: loop counters are not serialized, so `unsigned int` is appropriate. **No issue.**

9. **No blank line between declarations and statements** (main.c:251, etc.)
   DPDK style requires an empty line. Many functions have this. Spot check:
   - Line 251 in `recv_full()`: `uint8_t *p = buf;` followed immediately by `while (len > 0)`. **Violation.**
   - Line 885 in `read_filter()`: declarations end at line 892, code starts line 894. **Correct.**
   - Line