|WARNING| net/intel: fix use of non-recommended string functions

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch replaces unsafe string functions (`strncpy`, `strcpy`, `strcat`) with safer alternatives (`strlcpy`, `strlcat`). The changes are mostly correct, but there are several issues to address.

---

## Errors

### 1. `strlcat` on uninitialized buffer (ice_ethdev.c)

**File:** `drivers/net/intel/ice/ice_ethdev.c`  
**Lines:** 2057-2058, 2061-2062

The code uses `strlcat` to append to `pkg_file` after `strlcpy`, but `pkg_file` is a local variable that may contain uninitialized data beyond the null terminator written by `strlcpy`. While `strlcat` will work correctly (it scans for the null terminator), the buffer should be zero-initialized for clarity and to avoid undefined behavior if the buffer size checks fail.

```c
/* Current code */
strlcpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
strlcat(pkg_file, opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE);
```

**Issue:** If `strlcpy` truncates (returns >= `ICE_MAX_PKG_FILENAME_SIZE`), the subsequent `strlcat` operates on a truncated path, silently producing a wrong filename. Neither call checks the return value.

**Suggested fix:**

```c
char pkg_file[ICE_MAX_PKG_FILENAME_SIZE] = {0};
size_t len;

len = strlcpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES, sizeof(pkg_file));
if (len >= sizeof(pkg_file)) {
	PMD_INIT_LOG(ERR, "Package path too long");
	return -1;
}
len = strlcat(pkg_file, opt_ddp_filename, sizeof(pkg_file));
if (len >= sizeof(pkg_file)) {
	PMD_INIT_LOG(ERR, "Package filename too long");
	return -1;
}
```

**Same issue at lines 2061-2062, 2057-2058, and 2061-2062.** All four `strlcpy`+`strlcat` sequences need the same fix.

---

## Warnings

### 1. Missing return value checks on `strlcpy` (multiple files)

**Files:** `cpfl_ethdev.c`, `cpfl_flow_parser.c`, `i40e_ethdev.c`, `idpf_ethdev.c`, `ice_ethdev.c`

`strlcpy` returns the length of the source string. If the return value is >= the destination size, truncation occurred. While truncation may be acceptable in some contexts (e.g., logging), the code should verify that critical paths do not silently truncate identifiers or configuration strings.

**Examples:**

- `cpfl_ethdev.c:2537`: Device name truncation could cause confusion in logs or multi-device setups
- `cpfl_flow_parser.c:214, 719, 744, 812, 858, 1808`: Field name truncation could break flow rule matching
- `ice_ethdev.c:1900`: Driver version string truncation (less critical, but should be verified)
- `idpf_ethdev.c:1500`: Device name truncation

**Suggested approach:**

For critical paths (device names, flow rule field names), add a check:

```c
if (strlcpy(dst, src, sizeof(dst)) >= sizeof(dst)) {
	PMD_DRV_LOG(ERR, "String truncation: %s", src);
	return -EINVAL;  /* or goto err */
}
```

For non-critical paths (version strings, logging), the current code is acceptable but a comment explaining truncation is intentional would be helpful.

### 2. Redundant bounds check before `strlcpy`

**File:** `drivers/net/intel/cpfl/cpfl_flow_parser.c`  
**Lines:** 209-211, 714-716, 739-741, 807-809, 853-855

The code checks `strlen(name) >= CPFL_JS_STR_SIZE` before calling `strlcpy`. This check is redundant because `strlcpy` already handles this case by truncating and null-terminating. However, the current code returns an error on truncation, which is the safer behavior for field names that must match exactly.

**Current pattern:**

```c
if (strlen(name) >= CPFL_JS_STR_SIZE) {
	PMD_DRV_LOG(ERR, "The 'name' is too long.");
	goto err;
}
strlcpy(js_field->fields[i].name, name, CPFL_JS_STR_SIZE);
```

**This is actually correct.** The pre-check enforces exact match requirements. However, the pattern could be simplified by checking `strlcpy`'s return value instead:

```c
if (strlcpy(js_field->fields[i].name, name, CPFL_JS_STR_SIZE) >= CPFL_JS_STR_SIZE) {
	PMD_DRV_LOG(ERR, "The 'name' is too long: %s", name);
	goto err;
}
```

This is a **suggestion**, not a required change. The current code is correct.

### 3. Style: sizeof(array) vs constant

**Files:** `cpfl_flow_parser.c`, `ice_ethdev.c`

Some calls use `CPFL_JS_STR_SIZE` (the defined constant) while others could use `sizeof(destination)` for consistency with the rest of the patch. This is a minor style inconsistency.

**Example:**

```c
/* Current */
strlcpy(js_field->fields[i].name, name, CPFL_JS_STR_SIZE);

/* Alternative (more consistent with other changes in this patch) */
strlcpy(js_field->fields[i].name, name, sizeof(js_field->fields[i].name));
```

**This is acceptable either way.** Using the constant is fine if `CPFL_JS_STR_SIZE` matches the actual array size. No change required.

---

## Info

### 1. Removed `memset` before `strlcpy` (i40e_ethdev.c)

**File:** `drivers/net/intel/i40e/i40e_ethdev.c`  
**Line:** 11919

The patch removes `memset(name, 0, sizeof(name));` before `strlcpy`. This is correct because `strlcpy` null-terminates the destination, making the `memset` unnecessary. Good cleanup.

### 2. `strcpy` to `strlcpy` with `sizeof` destination (e1000, ixgbe)

**Files:** `e1000/igb_ethdev.c:2050-2052`, `ixgbe/ixgbe_ethdev.c:3638-3640`

The patch correctly changes `strcpy` to `strlcpy` and adds the destination size. The `sizeof(xstats_names[i].name)` argument is correct (it's an array member of a struct, not a pointer).

---

## Positive Observations

1. **Consistent pattern**: The patch uniformly replaces unsafe functions throughout the Intel driver subsystem
2. **Correct `sizeof` usage**: All changes use `sizeof(destination)` correctly, not `sizeof(pointer)`
3. **Maintains functionality**: No behavioral changes except improved safety
4. **Appropriate use of `strlcat`**: The `strcat` - `strlcat` changes are correct (ice_ethdev.c)

---

## Recommendations

1. **Critical (Error):** Add return value checks and error handling for the `strlcpy`+`strlcat` sequences in `ice_ethdev.c` (lines 2057-2062). Truncation here would cause incorrect file paths.

2. **Important (Warning):** Consider adding return value checks for `strlcpy` on device names (`cpfl_ethdev.c:2537`, `idpf_ethdev.c:1500`) and flow parser field names (`cpfl_flow_parser.c`). Truncation could cause subtle bugs.

3. **Optional (Info):** The patch is otherwise correct and ready after addressing the `strlcat` error handling.

---

## Severity Summary

- **Errors:** 1 (missing error handling for `strlcat` on potentially truncated paths)
- **Warnings:** 2 (missing return value checks in critical paths)
- **Info:** 2 (positive observations)

The patch is a valuable improvement to code safety. The primary concern is ensuring path construction in `ice_load_pkg` cannot silently produce wrong filenames.