On Fri, 22 Nov 2024 19:04:32 +0800 Jie Hai <[email protected]> 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: [email protected]` | ⚠️ 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: [email protected] | ✅ 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: [email protected]` | ⚠️ 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: [email protected]` since this is a bug fix.
