|WARNING| [v2] examples: use strlcpy and strlcat

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-24

# DPDK Patch Review

## Summary
This patch replaces unsafe string functions (`strncpy`, `strcpy`, `strcat`) with safer alternatives (`strlcpy`, `strlcat`, `memcpy`) in the examples directory. Overall, the changes improve safety, but there are issues with the `memcpy` conversions and missing header inclusion.

---

## Errors

### 1. `memcpy` does not guarantee null termination (sa.c)

**File:** `examples/ipsec-secgw/sa.c`

**Issue:** The patch replaces `strncpy` with `memcpy` for string operations, but `memcpy` does not null-terminate the destination. The original code relied on `strncpy` to null-pad or the fact that `sub_str` was zero-initialized, but switching to `memcpy` without ensuring null termination could lead to undefined behavior when `sub_str` is later used as a string.

**Lines:**
```c
-			strncpy(sub_str, pt_start, 2);
+			memcpy(sub_str, pt_start, 2);
```
and
```c
-			strncpy(sub_str, pt_start, pt_end - pt_start);
+			memcpy(sub_str, pt_start, pt_end - pt_start);
```

**Fix:** Use `strlcpy` instead of `memcpy` to ensure null termination, or manually null-terminate after `memcpy`:
```c
/* Option 1: Use strlcpy */
strlcpy(sub_str, pt_start, sizeof(sub_str));

/* Option 2: Manually null-terminate after memcpy */
size_t len = (pt_end == NULL) ? 2 : (pt_end - pt_start);
if (len >= sizeof(sub_str))
	return 0;
memcpy(sub_str, pt_start, len);
sub_str[len] = '\0';
```

**Note:** Without seeing the full context of how `sub_str` is declared and used, I cannot determine if the original code already had safeguards (e.g., `sub_str` declared as `char sub_str[3] = {0}`). However, `memcpy` is semantically wrong for string operations that require null termination.

---

### 2. Missing header inclusion (ip_pipeline/conn.c, pipeline/conn.c)

**File:** `examples/ip_pipeline/conn.c`, `examples/pipeline/conn.c`

**Issue:** The patch adds `#include <rte_string_fns.h>` to provide `strlcpy`, which is correct for portability. However, verify that this header exists and is available in all supported DPDK versions. The commit message mentions "older distros without a built-in strlcpy fn", which suggests this is intentional.

**Status:** This appears correct based on the v2 changelog. No action needed, but confirm `rte_string_fns.h` is the correct header in the DPDK version being targeted.

---

## Warnings

### 1. Potential buffer size mismatch (conn.c files)

**File:** `examples/ip_pipeline/conn.c`, `examples/pipeline/conn.c`

**Lines:**
```c
-	strncpy(conn->welcome, p->welcome, CONN_WELCOME_LEN_MAX);
-	strncpy(conn->prompt, p->prompt, CONN_PROMPT_LEN_MAX);
+	strlcpy(conn->welcome, p->welcome, CONN_WELCOME_LEN_MAX + 1);
+	strlcpy(conn->prompt, p->prompt, CONN_PROMPT_LEN_MAX + 1);
```

**Issue:** The original `strncpy` uses `CONN_WELCOME_LEN_MAX` as the count, which typically writes up to `CONN_WELCOME_LEN_MAX` bytes (not including null terminator if the source is longer). The patch changes to `CONN_WELCOME_LEN_MAX + 1`, which is correct for `strlcpy`'s size parameter (the total buffer size including null terminator). However, this assumes the destination buffers (`conn->welcome`, `conn->prompt`) are actually sized as `CONN_WELCOME_LEN_MAX + 1` or larger.

**Verification needed:** Check the declaration of `conn->welcome` and `conn->prompt` to confirm they are sized appropriately:
- If declared as `char welcome[CONN_WELCOME_LEN_MAX]`, then using `CONN_WELCOME_LEN_MAX + 1` as the `strlcpy` size is **wrong** (buffer overflow).
- If declared as `char welcome[CONN_WELCOME_LEN_MAX + 1]`, then the change is correct.

**Suggested fix (if buffers are sized `CONN_WELCOME_LEN_MAX`):**
```c
strlcpy(conn->welcome, p->welcome, CONN_WELCOME_LEN_MAX);
strlcpy(conn->prompt, p->prompt, CONN_PROMPT_LEN_MAX);
```

Without access to the structure definitions, I flag this as a potential off-by-one error. The reviewer must verify the buffer sizes.

---

### 2. Inconsistent conversion strategy (sa.c)

**File:** `examples/ipsec-secgw/sa.c`

**Issue:** The patch converts two `strncpy` calls to `memcpy`, while all other conversions in the patch use `strlcpy` or `strlcat`. This inconsistency suggests either:
1. The `sa.c` changes are operating on binary data (not strings), or
2. The conversion is incorrect and should use `strlcpy`.

Given that the code is parsing a key string and extracting hex digit pairs, the data is likely binary after parsing, but the intermediate `sub_str` buffer appears to be used as a string (e.g., passed to `strtol` or similar). Confirm the intended semantics.

**Recommendation:** If `sub_str` is used as a string, use `strlcpy` with proper bounds checking. If it's binary data, document why `memcpy` is appropriate and ensure null termination is not expected.

---

## Info

### 1. `strlcat` size parameter correctness (cli.c)

**File:** `examples/pipeline/cli.c`

**Lines:**
```c
-		strcat(line, " ");
+		strlcat(line, " ", MAX_LINE_SIZE);

-	strcat(line, tokens[i]);
+	strlcat(line, tokens[i], MAX_LINE_SIZE);
```

**Observation:** The conversion to `strlcat` is correct. The size parameter `MAX_LINE_SIZE` should be the total size of the `line` buffer. Verify that `line` is declared as `char line[MAX_LINE_SIZE]` or larger.

**No issue** if the buffer size matches the constant.

---

### 2. Original `strncpy` already correct in vm_power_manager (channel_manager.c)

**File:** `examples/vm_power_manager/channel_manager.c`

**Original code:**
```c
strncpy(new_domain->name, vm_name, sizeof(new_domain->name));
new_domain->name[sizeof(new_domain->name) - 1] = '\0';
```

**New code:**
```c
strlcpy(new_domain->name, vm_name, sizeof(new_domain->name));
```

**Observation:** The original code was already safe (manually null-terminating after `strncpy`). The conversion to `strlcpy` is an improvement (more concise), but not a bug fix. This is fine.

---

## Summary of Required Actions

1. **Fix `memcpy` in sa.c** (Error): Replace with `strlcpy` or ensure null termination manually.
2. **Verify buffer sizes in conn.c** (Warning): Confirm that `CONN_WELCOME_LEN_MAX + 1` does not exceed the actual buffer size.
3. **Clarify `memcpy` usage** (Warning): Document why `memcpy` is used in `sa.c` if it's intentional, or convert to `strlcpy`.

All other changes appear correct.