Re: [PATCH v6 00/25] replace strtok with strtok_r

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 22 Nov 2024 19:04:32 +0800
Jie Hai <haijie1@huawei.com> wrote:

> Multiple threads calling the same function may cause condition
> race issues, which often leads to abnormal behavior and can cause
> more serious vulnerabilities such as abnormal termination, denial
> of service, and compromised data integrity.
> 
> This patchset replaces strtok with strtok_r in app, example, lib
> and drivers. And adds check for use of strtok in checkpatches.sh.
> 
> --
> v6:
> 1. adapt to the newest codes.
> 2. fix compile error.
> 
> v5:
> 1. remove CC stable for some patch.
> 2. replace strtok for all files.
> 
> v4:
> 1. fix mispellings.
> 2. add Acked-bys and Reviewd-bys.
> 3. remove some patch and add new. 
> v3:
> 1. fix compile error.
> 2. use strtok_r instead.
> v2:
> 1. fix commit log.
> 2. add check in checkpatches.sh.
> 3. replace strtok_r with strtok_s.
> 4. add Acked-by.
> --
> 
> Jie Hai (25):
>   app/bbdev: replace strtok with reentrant version
>   app/compress-perf: replace strtok with reentrant version
>   app/crypto-perf: replace strtok with reentrant version
>   app/dma-perf: replace strtok with reentrant version
>   app/flow-perf: replace strtok with reentrant version
>   app/test-mldev: replace strtok with reentrant version
>   app/test-fib: replace strtok with reentrant version
>   dmadev: replace strtok with reentrant version
>   eal: replace strtok with reentrant version
>   ethdev: replace strtok with reentrant version
>   eventdev: replace strtok with reentrant version
>   security: replace strtok with reentrant version
>   telemetry: replace strtok with reentrant version
>   bus/fslmc: replace strtok with reentrant version
>   common/cnxk: replace strtok with reentrant version
>   event/cnxk: replace strtok with reentrant version
>   net/ark: replace strtok with reentrant version
>   raw/cnxk_gpio: replace strtok with reentrant version
>   net/cnxk: replace strtok with reentrant version
>   common/qat: replace strtok with reentrant version
>   net/mlx5: replace strtok with reentrant version
>   examples/l2fwd-crypto: replace strtok with reentrant version
>   examples/vhost: replace strtok with reentrant version
>   devtools: check for some reentrant function
>   eal/linux: install rte_os_shim.h file
> 
>  app/test-bbdev/test_bbdev_vector.c            | 42 +++++++++++--------
>  .../comp_perf_options_parse.c                 | 17 ++++----
>  app/test-crypto-perf/cperf_options_parsing.c  | 17 ++++----
>  .../cperf_test_vector_parsing.c               | 11 +++--
>  app/test-dma-perf/main.c                      |  9 ++--
>  app/test-fib/main.c                           | 11 ++---
>  app/test-flow-perf/main.c                     | 23 +++++-----
>  app/test-mldev/ml_options.c                   | 19 +++++----
>  devtools/checkpatches.sh                      |  8 ++++
>  drivers/bus/fslmc/fslmc_bus.c                 |  6 ++-
>  drivers/bus/fslmc/portal/dpaa2_hw_dpio.c      |  5 ++-
>  drivers/common/cnxk/cnxk_telemetry_nix.c      | 13 +++---
>  drivers/common/qat/qat_device.c               |  6 ++-
>  drivers/event/cnxk/cnxk_eventdev.c            | 12 ++++--
>  drivers/event/cnxk/cnxk_tim_evdev.c           | 12 +++---
>  drivers/net/ark/ark_pktchkr.c                 | 11 ++---
>  drivers/net/ark/ark_pktgen.c                  | 11 ++---
>  drivers/net/cnxk/cnxk_ethdev_sec_telemetry.c  |  6 ++-
>  drivers/net/mlx5/mlx5_testpmd.c               |  5 ++-
>  drivers/raw/cnxk_gpio/cnxk_gpio.c             |  7 ++--
>  examples/l2fwd-crypto/main.c                  |  7 ++--
>  examples/vhost/main.c                         |  4 +-
>  lib/dmadev/rte_dmadev.c                       |  5 ++-
>  lib/eal/common/eal_common_memory.c            |  8 ++--
>  lib/eal/linux/include/meson.build             |  1 +
>  lib/ethdev/rte_ethdev_telemetry.c             | 10 +++--
>  lib/eventdev/rte_event_eth_rx_adapter.c       | 39 ++++++++---------
>  lib/eventdev/rte_eventdev.c                   | 18 ++++----
>  lib/security/rte_security.c                   |  4 +-
>  lib/telemetry/telemetry.c                     |  6 ++-
>  30 files changed, 208 insertions(+), 145 deletions(-)
> 


This patch series seems to have gotten lost or stalled.
Still worth fixing strtok. But it no longer merges with latest code base.
Also AI code review sees a bug that needs fixing.


## DPDK Patch Evaluation Report

**Series**: v6 strtok → strtok_r conversion (patches 1/25, 24/25, 25/25 from bundle)

---

### Patch 1/25: `app/bbdev: replace strtok with reentrant version`

#### Commit Message Analysis

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 47 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | "replace" is imperative |
| No trailing period | ✅ PASS | |
| Correct prefix | ⚠️ INFO | Uses `app/bbdev:` for `app/test-bbdev/` — acceptable shorthand |
| Body ≤75 chars/line | ✅ PASS | |
| Body doesn't start with "It" | ✅ PASS | Starts with "Multiple" |
| Signed-off-by present | ✅ PASS | Real name, valid email |
| Fixes: tag format | ✅ PASS | 12-char SHA with quoted subject |
| Tag order | ✅ PASS | Fixes → blank line → Signed-off-by → Acked-by |

#### Warnings

| Issue | Severity | Recommendation |
|-------|----------|----------------|
| Missing `Cc: stable@dpdk.org` | ⚠️ WARNING | Bug fix with `Fixes:` tags should include stable backport CC |

#### Code Quality — **CRITICAL BUG FOUND**

**Error**: Incorrect `strtok_r()` usage in multiple locations.

The patch incorrectly replaces `strtok(NULL, ...)` with `strtok_r(token, ...)` instead of `strtok_r(NULL, ...)` for continuation calls:

```c
/* Lines 187-188: BUG - should be NULL, not token */
-               tok = strtok(NULL, VALUE_DELIMITER);
+               tok = strtok_r(token, VALUE_DELIMITER, &sp);  // WRONG!
                                ^^^^^ should be NULL
```

This error appears in at least 4 locations:
- Line 188: `cs_theta_0` parsing loop
- Line 203: `cs_theta_d` parsing loop  
- Line 218: `time_offset` parsing loop
- Line 234: `fft_window_width` parsing loop

**Correct pattern**:
```c
tok = strtok_r(token, VALUE_DELIMITER, &sp);  /* Initial call: pass string */
/* ... */
tok = strtok_r(NULL, VALUE_DELIMITER, &sp);   /* Subsequent calls: pass NULL */
```

This bug would cause the parser to restart from the beginning of `token` on each iteration instead of continuing through the string, likely resulting in infinite loops or incorrect data parsing.

#### Other Code Observations

| Check | Status |
|-------|--------|
| `#include <rte_os_shim.h>` added | ✅ Correct |
| State variable `char *sp = NULL` declared | ✅ Correct |
| First strtok_r calls pass string | ✅ Correct |

---

### Patch 24/25: `devtools: check for some reentrant function`

#### Commit Message Analysis

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 42 characters |
| Correct prefix | ✅ PASS | `devtools:` |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| No trailing period | ✅ PASS | |
| Signed-off-by | ✅ PASS | |
| Cc: stable@dpdk.org | ✅ PASS | Present |
| Tag order | ✅ PASS | Cc → blank → Signed-off-by → Acked-by |

#### Code Review

| Check | Status |
|-------|--------|
| Shell script syntax | ✅ Correct |
| awk pattern for `strtok\(` | ✅ Correct |
| Consistent with existing checks | ✅ Follows established pattern |

**No issues found** — this patch looks correct.

---

### Patch 25/25: `eal/linux: install rte_os_shim.h file`

#### Commit Message Analysis

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 35 characters |
| Correct prefix | ✅ PASS | `eal/linux:` for `lib/eal/linux/` |
| Imperative mood | ✅ PASS | |
| Signed-off-by | ✅ PASS | |

#### Warnings

| Issue | Severity | Notes |
|-------|----------|-------|
| Missing `Cc: stable@dpdk.org` | ⚠️ WARNING | This is a build fix; if backporting the strtok_r changes, this would also be needed |
| No `Fixes:` tag | ℹ️ INFO | Could reference the commit that exposed the missing header |

#### Code Review

| Check | Status |
|-------|--------|
| Meson file syntax | ✅ Correct |
| Alphabetical ordering | ✅ Maintained (rte_os.h → rte_os_shim.h) |
| Trailing comma | ✅ Correct for multi-line list |

---

## Summary

| Patch | Verdict | Blocking Issues |
|-------|---------|-----------------|
| 01/25 app/bbdev | ❌ **NEEDS REVISION** | Critical bug: `strtok_r(token,...)` should be `strtok_r(NULL,...)` for continuation calls |
| 24/25 devtools | ✅ **ACCEPTABLE** | None |
| 25/25 eal/linux | ✅ **ACCEPTABLE** | Minor: consider adding `Cc: stable` |

### Required Actions

1. **Patch 1/25**: Fix all `strtok_r()` continuation calls to pass `NULL` instead of `token`:
   ```c
   /* Wrong */
   tok = strtok_r(token, VALUE_DELIMITER, &sp);
   
   /* Correct */
   tok = strtok_r(NULL, VALUE_DELIMITER, &sp);
   ```

2. **Patch 1/25**: Consider adding `Cc: stable@dpdk.org` since this is a bug fix.