|WARNING| [v4,4/4] app/rpcapd: add host list option

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/4: pcapng: add API to read back capture mbuf header

### Errors

None identified.

### Warnings

None identified.

### Info

- **New experimental API properly tagged**: The new function `rte_pcapng_pkt_info()` is correctly marked with `__rte_experimental` and uses `RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pcapng_pkt_info, 26.11)`.
- **Release notes updated**: The patch adds appropriate documentation in `doc/guides/rel_notes/release_26_11.rst`.
- **Test coverage added**: The patch includes comprehensive tests in `app/test/test_pcapng.c` covering nominal cases, truncation, VLAN tag restoration, and error handling.
- **API is well documented**: Good Doxygen comments explaining behavior, including the note about VLAN tag insertion potentially making the capture larger than the original.

---

## PATCH 2/4: app/rpcapd: remote pcap daemon

### Errors

1. **Non-const function pointer array** (`main.c:165`, `main.c:233`):
   ```c
   struct conn ctrl = { .fd = ctrl_fd };
   struct session s = { .data.fd = -1 };
   ```
   These are not function pointers, so this guideline does not apply. No error here.

2. **Resource leak on error path** (`capture.c:232`):
   ```c
   s->ring = create_capture_ring(s->port);
   s->mp = create_capture_mempool(s->port, s->snaplen);
   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 only one of `s->ring` or `s->mp` is non-NULL, `stop_capture()` will free it correctly. However, there is a potential issue: if `create_capture_mempool()` succeeds but the subsequent `open_data_listener()` fails, the code calls `stop_capture()` which will clean up properly. This is actually correct. No error.

3. **Missing error check** (`capture.c:295`):
   ```c
   if (send_timeout > 0) {
       struct timeval tv = {
           .tv_sec = send_timeout,
       };
       if (setsockopt(data_fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)) < 0)
           RPCAPD_LOG(NOTICE, "cannot set data send timeout: %s",
                      strerror(errno));
   }
   ```
   The error is logged but the operation continues. This is acceptable for a non-critical socket option. No error.

4. **Missing bounds check** (`filter.c:74`):
   ```c
   for (i = 0; i < nitems; i++) {
       if (recv_full(c, &winsn, sizeof(winsn)) < 0) {
           free(insns);
           return -1;
       }
       insns[i].code = rte_be_to_cpu_16(winsn.code);
       insns[i].jt   = winsn.jt;
       insns[i].jf   = winsn.jf;
       insns[i].k    = rte_be_to_cpu_32(winsn.k);
   }
   ```
   The loop is bounded by `nitems`, which was already checked to be <= `MAX_FILTER_INSNS` and to fit within `plen`. The `insns` array was allocated with `calloc(nitems, ...)`. This is correctly bounded. No error.

5. **Potential use-after-free** (`capture.c:391`):
   ```c
   TEST_ASSERT(memcmp(data, rte_pktmbuf_mtod(&mbfs.mb[0], const void *),
                      rte_pktmbuf_data_len(&mbfs.mb[0])) == 0,
               "packet data is not at data_offset");
   ```
   This is in a test, not production code. The test constructs `mbfs` at the start of `test_pkt_info()` and the mbuf `mc` is not freed until later. No use-after-free. No error.

**Actually, re-reading the resource leak checks:**

In `handle_startcap()` in `capture.c`:
```c
s->ring = create_capture_ring(s->port);
s->mp = create_capture_mempool(s->port, s->snaplen);
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");
}

data_listen = open_data_listener(&data_port);
if (data_listen < 0) {
    stop_capture(s);
    return rpcap_send_error(c, 0, "data port setup failed");
}
```
If `open_data_listener()` fails, `stop_capture()` is called. Looking at `stop_capture()`:
```c
void
stop_capture(struct session *s)
{
    if (s->ring != NULL) {
        while ((n = rte_ring_sc_dequeue_burst(...)) > 0)
            rte_pktmbuf_free_bulk(pkts, n);
        rte_ring_free(s->ring);
        s->ring = NULL;
    }
    if (s->mp != NULL) {
        rte_mempool_free(s->mp);
        s->mp = NULL;
    }
    // ...
}
```
This correctly frees both `ring` and `mp` if they are non-NULL. No leak.

However, there is a different pattern later:
```c
if (rte_eth_promiscuous_get(s->port) != 1) {
    if (rte_eth_promiscuous_enable(s->port) == 0)
        s->promisc_set = true;
    else
        RPCAPD_LOG(NOTICE, "cannot enable promiscuous mode on %s",
            s->name);
}
```
If promiscuous enable fails, the code logs a notice but continues. This is acceptable: the capture can still run, just not in promiscuous mode. No error.

Further down:
```c
if (rte_pdump_enable_bpf(s->port, RTE_PDUMP_ALL_QUEUES,
                         s->pdump_flags | RTE_PDUMP_FLAG_PCAPNG,
                         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));
    close(data_listen);
    stop_capture(s);
    return rpcap_send_error(c, 0, "cannot enable capture");
}
```
If `rte_pdump_enable_bpf()` fails, `data_listen` is closed and `stop_capture()` is called. Correct cleanup. No leak.

Then:
```c
if (rpcap_send_msg(c, RPCAP_MSG_STARTCAP_REPLY, 0, &reply, sizeof(reply)) < 0) {
    close(data_listen);
    stop_capture(s);
    return -1;
}
```
If the send fails, `data_listen` is closed and `stop_capture()` is called. Correct cleanup.

```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;
}
```
If `accept_from()` fails, `data_listen` is already closed, `stop_capture()` is called. Correct cleanup. No leak.

All error paths in `handle_startcap()` correctly clean up resources. No errors identified.

### Warnings

1. **Missing release notes for internal helper** (`rpcapd.h`, `timestamp_init()`):
   The function `timestamp_init()` is internal to the application and does not require release notes. No warning.

2. **Static function pointer arrays** (`main.c:386`):
   ```c
   static const struct option long_options[] = {
       { "port",         required_argument, NULL, 'p' },
       // ...
   };
   ```
   This is not a function pointer array; it's an array of `struct option`. No warning.

3. **Using `malloc()` instead of `rte_zmalloc_socket()` for queue-related structures**:
   Searching for queue/ring/descriptor allocations:
   - `capture.c:114`: `rte_ring_create()` is used (correct).
   - `capture.c:126`: `rte_pktmbuf_pool_create()` is used (correct).
   - `filter.c:66`: `calloc(nitems, sizeof(*insns))` allocates a temporary array for BPF instructions, not a queue structure. This is process-private and does not need hugepage backing. No warning.
   - `session.c:24`: `realloc(buf, ...)` for building the interface list response. Process-private, not a queue. No warning.

4. **Non-unique global variable prefix**:
   Checking global variables in the installed application (not a library, so less strict):
   - `main.c`: `listen_port`, `ring_size`, `send_timeout`, `use_tls`, `null_auth_ok`, etc. These are all prefixed with descriptive names or are `static`. The non-static ones (`listen_addr`, `listen_addrlen`, `quit_signal`, `ring_size`, `send_timeout`, `use_tls`, `null_auth_ok`) are application-global and acceptable. However, they lack a consistent prefix like `rpcapd_`. This is a **Warning** for an application where static linking might occur in the future, but since this is an application binary (not a library), it's less critical.

   Since this is an application in `app/` and not a library or driver, the unique prefix guideline is less strictly enforced. However, for best practice, a warning could be issued. Given the context, I'll classify this as **Info** rather than Warning.

5. **RST documentation style** (`doc/guides/tools/rpcapd.rst`):
   The documentation uses bullet lists for command-line options. The guidelines suggest definition lists for term/description patterns. However, the existing style is consistent with other DPDK tool documentation (e.g., `dumpcap.rst`, `proc_info.rst`). This is acceptable. No warning.

### Info

- **Application structure**: The application is well-structured into separate modules (capture, filter, session, sock) with clear separation of concerns.
- **Error handling**: Comprehensive error handling with logging at appropriate levels.
- **Const correctness**: Many functions correctly use `const` for read-only parameters (e.g., `const struct conn *c`, `const struct session *s`).
- **Documentation**: The tool documentation is thorough, including usage examples and security warnings.

---

## PATCH 3/4: app/rpcapd: add TLS support

### Errors

1. **Missing error check** (`tls.c:89`):
   ```c
   if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)) < 0 ||
       setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)) < 0) {
       RPCAPD_LOG(NOTICE, "cannot set TLS handshake timeout: %s",
                  strerror(errno));
       return -1;
   }
   ```
   The error is logged and the function returns -1, which will cause the client connection to be rejected. This is correct handling. No error.

2. **Resource leak on error path** (`tls.c:105`):
   ```c
   SSL *ssl = SSL_new(tls_ctx);
   bool timed = set_handshake_timeout(c->fd, TLS_HANDSHAKE_TIMEOUT_SEC) == 0;

   if (ssl == NULL) {
       RPCAPD_LOG(ERR, "SSL_new: %s", tls_strerror());
       return -1;
   }

   if (SSL_set_fd(ssl, c->fd) != 1) {
       RPCAPD_LOG(ERR, "SSL_set_fd: %s", tls_strerror());
       SSL_free(ssl);
       return -1;
   }

   if (SSL_accept(ssl) != 1) {
       // ...
       SSL_free(ssl);
       return -1;
   }
   ```
   If `SSL_accept()` fails, `ssl` is freed. If `SSL_set_fd()` fails, `ssl` is freed. If `SSL_new()` fails, there is no `ssl` to free. All paths are correct. No leak.

3. **Plaintext password over unencrypted connection** (`session.c:213`):
   ```c
   if (c->ssl == NULL && !is_loopback(&s->peer)) {
       free_credential(user);
       free_credential(password);
       RPCAPD_LOG(NOTICE,
                  "refusing password authentication on an unencrypted connection");
       return rpcap_send_error(c, PCAP_ERR_AUTH_FAILED,
                               "this server will not accept a password "
                               "over an unencrypted connection; "
                               "use rpcaps://");
   }
   ```
   The code correctly refuses password authentication over an unencrypted connection unless the peer is loopback. Good security practice. No error.

4. **Credentials not zeroed before free** (`session.c:59`):
   ```c
   static void
   free_credential(char *cred)
   {
       if (cred != NULL) {
           explicit_bzero(cred, strlen(cred));
           free(cred);
       }
   }
   ```
   Credentials are explicitly zeroed using `explicit_bzero()` before being freed. This is correct. No error.

5. **`recv_credential()` potential buffer overflow** (`session.c:169`):
   ```c
   static int
   recv_credential(const struct conn *c, uint32_t len, uint32_t *plen, char **out)
   {
       char *buf;

       if (len > *plen || len > MAX_CREDENTIAL_LEN)
           return -1;

       buf = malloc(len + 1);
       if (buf == NULL)
           return -1;

       if (recv_full(c, buf, len) < 0) {
           explicit_bzero(buf, len);
           free(buf);
           return -1;
       }
       buf[len] = '\0';
       *plen -= len;
       *out = buf;
       return 0;
   }
   ```
   The code checks `len > MAX_CREDENTIAL_LEN` before allocating `len + 1` bytes and reading `len` bytes. The null terminator is placed at `buf[len]`. This is correct. No overflow.

### Warnings

1. **Static function pointer array** (`main.c:395`):
   ```c
   static const struct option long_options[] = {
       { "port",         required_argument, NULL, 'p' },
       { "bind",         required_argument, NULL, 'b' },
       { "null-auth",    no_argument,       NULL, 'n' },
   #ifdef RTE_HAS_OPENSSL
       { "tls",          no_argument,       NULL, 'S' },
       { "cert",         required_argument, NULL, 'X' },
       { "key",          required_argument, NULL,