|WARNING| examples: use strlcpy and strlcat

[dpdklab at iol.unh.edu]

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

_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 their safer alternatives (`strlcpy`, `strlcat`) across several example applications. Overall, the changes improve string handling safety by guaranteeing null termination.

---

## Errors

**1. `strlcpy()` size argument incorrect (examples/ip_pipeline/conn.c, examples/pipeline/conn.c)**

The `strlcpy()` calls use `CONN_WELCOME_LEN_MAX + 1` and `CONN_PROMPT_LEN_MAX + 1` as the size argument, but this is only correct if the destination buffers are actually sized `[CONN_WELCOME_LEN_MAX + 1]`. If the buffers are sized `[CONN_WELCOME_LEN_MAX]`, this passes a size larger than the buffer, which could allow buffer overflow.

**Locations:**
- `examples/ip_pipeline/conn.c:118-119`
- `examples/pipeline/conn.c:119-120`

**Fix:** Verify the actual buffer sizes in the `conn` structure definition. If `conn->welcome` and `conn->prompt` are `char[CONN_WELCOME_LEN_MAX]`, use `CONN_WELCOME_LEN_MAX` (not `+ 1`) as the size argument:

```c
strlcpy(conn->welcome, p->welcome, CONN_WELCOME_LEN_MAX);
strlcpy(conn->prompt, p->prompt, CONN_PROMPT_LEN_MAX);
```

If the buffers are `char[CONN_WELCOME_LEN_MAX + 1]`, the patch is correct as-is.

---

**2. `strlcat()` concatenation to uninitialized buffer (examples/pipeline/cli.c)**

The code initializes `line[0] = 0` then uses `strlcat()` to append tokens. However, `strlcat()` expects the destination string to already be null-terminated and calculates the append position by scanning for the existing null terminator. This pattern works, but the first call should use `strlcpy()` (not `strlcat()`) for the first token, or the buffer should be explicitly zero-initialized.

**Location:** `examples/pipeline/cli.c:172-178`

**Current code:**
```c
line[0] = 0;
for (i = 0; i < n_tokens; i++) {
    if (i)
        strlcat(line, " ", MAX_LINE_SIZE);
    
    strlcat(line, tokens[i], MAX_LINE_SIZE);
}
```

**Issue:** The first `strlcat(line, tokens[0], MAX_LINE_SIZE)` call (when `i == 0`) scans for a null terminator starting at `line[0]`, which is `'\0'`. This works, but is less efficient and less clear than using `strlcpy()` for the first token.

**Suggested fix:**
```c
line[0] = '\0';
for (i = 0; i < n_tokens; i++) {
    if (i == 0)
        strlcpy(line, tokens[i], MAX_LINE_SIZE);
    else {
        strlcat(line, " ", MAX_LINE_SIZE);
        strlcat(line, tokens[i], MAX_LINE_SIZE);
    }
}
```

Or, more concisely:
```c
snprintf(line, MAX_LINE_SIZE, "%s", tokens[0]);
for (i = 1; i < n_tokens; i++) {
    strlcat(line, " ", MAX_LINE_SIZE);
    strlcat(line, tokens[i], MAX_LINE_SIZE);
}
```

---

**3. Missing buffer overflow check in `strlcat()` calls (examples/pipeline/cli.c)**

`strlcat()` returns the total length of the string it tried to create (not the amount appended). The caller should check the return value to detect truncation. If `strlcat()` returns a value >= the buffer size, the string was truncated, which could cause silent data loss or malformed command strings.

**Location:** `examples/pipeline/cli.c:175, 177`

**Fix:** Check `strlcat()` return values:
```c
if (strlcat(line, " ", MAX_LINE_SIZE) >= MAX_LINE_SIZE ||
    strlcat(line, tokens[i], MAX_LINE_SIZE) >= MAX_LINE_SIZE) {
    /* Handle error: line too long */
    return -EINVAL; /* or other appropriate error */
}
```

---

## Warnings

**1. `memcpy()` used where `strlcpy()` would be safer (examples/ipsec-secgw/sa.c)**

The patch replaces `strncpy()` with `memcpy()` for copying substring components of a hex key string. While `memcpy()` avoids the `strncpy()` null-padding behavior, it does NOT null-terminate the destination. The subsequent code uses `strtol(sub_str, ...)` which expects a null-terminated string. The buffer `sub_str` is not shown in the patch context, but if it is not explicitly null-terminated after the `memcpy()`, this will cause undefined behavior.

**Location:** `examples/ipsec-secgw/sa.c:341, 346`

**Current code:**
```c
if (pt_end == NULL) {
    if (strlen(pt_start) > 2)
        return 0;
    memcpy(sub_str, pt_start, 2);
} else {
    if (pt_end - pt_start > 2)
        return 0;
    
    memcpy(sub_str, pt_start, pt_end - pt_start);
    pt_start = pt_end + 1;
}
```

**Issue:** `sub_str` is not null-terminated after `memcpy()`. If `sub_str` is later passed to `strtol()` or similar, undefined behavior.

**Suggested fix:** Use `strlcpy()` instead of `memcpy()`, or manually null-terminate:
```c
if (pt_end == NULL) {
    if (strlen(pt_start) > 2)
        return 0;
    strlcpy(sub_str, pt_start, sizeof(sub_str));
} else {
    size_t len = pt_end - pt_start;
    if (len > 2)
        return 0;
    memcpy(sub_str, pt_start, len);
    sub_str[len] = '\0';
    pt_start = pt_end + 1;
}
```

Alternatively, if `sub_str` is a 3-byte buffer (e.g., `char sub_str[3]`), the null terminator placement must be verified.

---

**2. `sizeof()` operator on array argument vs. buffer (examples/l2fwd-crypto/main.c)**

The patch uses `sizeof(string_cipher_op)` as the size argument to `strlcpy()`. This is correct only if `string_cipher_op` is a local array (not a pointer parameter). Since the variables are declared as `char string_cipher_op[MAX_STR_LEN]`, this usage is correct.

**Location:** `examples/l2fwd-crypto/main.c:1579-1591`

**No action needed** (this is flagged for verification only).

---

## Overall Assessment

The patch improves string safety by replacing unsafe functions with `strlcpy()` and `strlcat()`. However, three critical issues must be addressed:

1. **Incorrect buffer sizes** in `ip_pipeline/conn.c` and `pipeline/conn.c` (Error #1)
2. **Suboptimal pattern** in `pipeline/cli.c` using `strlcat()` on empty string (Error #2)
3. **Missing overflow checks** in `pipeline/cli.c` (Error #3)
4. **`memcpy()` without null termination** in `ipsec-secgw/sa.c` (Warning #1)

The first three are correctness bugs that must be fixed. The fourth is a potential correctness bug depending on how `sub_str` is used after the `memcpy()` (the patch context does not show this).