[PATCH 00/21] replace strtok with strtok_r

[fengchengwen]

On 2023/11/15 1:49, Tyler Retzlaff wrote:
> On Tue, Nov 14, 2023 at 09:34:33AM -0800, Tyler Retzlaff wrote:
>> On Tue, Nov 14, 2023 at 09:32:48AM -0800, Tyler Retzlaff wrote:
>>> On Tue, Nov 14, 2023 at 08:50:17PM +0800, Jie Hai wrote:
>>>> On 2023/11/14 1:09, Tyler Retzlaff wrote:
>>>>> On Mon, Nov 13, 2023 at 06:45:29PM +0800, Jie Hai 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.
>>>>>>
>>>>>> The strtok() is non-reentrant, it is better to replace it with a
>>>>>> reentrant function.
>>>>>
>>>>> could you please use strtok_s instead of strtok_r the former is part of
>>>>> the C11 standard the latter is not.
>>>>>
>>>>> thanks!
>>>>>
>>>> Hi, Tyler Retzlaff
>>>>
>>>> Thanks for your comment.
>>>>
>>>> For the use of strtok_s, I have consulted some documents, see
>>>> 	https://en.cppreference.com/w/c/string/byte/strtok
>>>> It says
>>>> "As with all bounds-checked functions, strtok_s only guaranteed to be
>>>> available if __STDC_LIB_EXT1__ is defined by the implementation and if
>>>> the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before
>>>> including <string.h>.
>>>> "
>>>>
>>>> I use strtok_s with "#ifdef __STDC_LIB_EXT1__ ... #endif" around and
>>>> compiled
>>>> locally and found that __STDC_LIB_EXT1__ was not defined, so the related
>>>> code  was not called. I'm afraid there's a problem with this
>>>> modification.
>>>>
>>>> Am I using strtok_s wrong?
>>>
>>> no i overlooked that it is optional my fault sorry.
>>>
>>> since there is no portable strtok_r i'm going to have to agree with others
>>> that only places where you actually need reentrant strtok be converted to
>>> strtok_r.
>>>
>>> for windows i think we'll either need to introduce an abstracted strtok_r
>>> name for portability or something in the rte_ namespace (dependent on
>>> what other revieweres would prefer)
>>
>> just as a follow up here maybe it would be optimal if we could use
>> strtok_s *if* we test that the toolchain makes it available and if not
>> provide a mapping of strtok_s -> strtok_r.
> 
> just a final follow up, i can see that we already have a rte_strerror
> here to do the replace with reentrant dance. it is probably good to
> follow the already established pattern for this and have a rte_strtok.

+1 for have rte_strtok which could cover different platform.

> 
> what do others think?
>  
>>>
>>> thanks!
>>>
>>>>
>>>> Thanks,
>>>> Jie Hai
>>>>>>
>>>>>> Jie Hai (21):
>>>>>>   app/graph: replace strtok with strtok_r
>>>>>>   app/test-bbdev: replace strtok with strtok_r
>>>>>>   app/test-compress-perf: replace strtok with strtok_r
>>>>>>   app/test-crypto-perf: replace strtok with strtok_r
>>>>>>   app/test-dma-perf: replace strtok with strtok_r
>>>>>>   app/test-fib: replace strtok with strtok_r
>>>>>>   app/dpdk-test-flow-perf: replace strtok with strtok_r
>>>>>>   app/test-mldev: replace strtok with strtok_r
>>>>>>   lib/dmadev: replace strtok with strtok_r
>>>>>>   lib/eal: replace strtok with strtok_r
>>>>>>   lib/ethdev: replace strtok with strtok_r
>>>>>>   lib/eventdev: replace strtok with strtok_r
>>>>>>   lib/telemetry: replace strtok with strtok_r
>>>>>>   lib/telemetry: replace strtok with strtok_r
>>>>>>   bus/fslmc: replace strtok with strtok_r
>>>>>>   common/cnxk: replace strtok with strtok_r
>>>>>>   event/cnxk: replace strtok with strtok_r
>>>>>>   net/ark: replace strtok with strtok_r
>>>>>>   raw/cnxk_gpio: replace strtok with strtok_r
>>>>>>   examples/l2fwd-crypto: replace strtok with strtok_r
>>>>>>   examples/vhost: replace strtok with strtok_r
>>>>>>
>>>>>>  app/graph/graph.c                             |  5 ++-
>>>>>>  app/graph/utils.c                             | 15 +++++---
>>>>>>  app/test-bbdev/test_bbdev_vector.c            | 25 +++++++-----
>>>>>>  .../comp_perf_options_parse.c                 | 16 ++++----
>>>>>>  app/test-crypto-perf/cperf_options_parsing.c  | 16 ++++----
>>>>>>  .../cperf_test_vector_parsing.c               | 10 +++--
>>>>>>  app/test-dma-perf/main.c                      | 13 ++++---
>>>>>>  app/test-fib/main.c                           | 10 ++---
>>>>>>  app/test-flow-perf/main.c                     | 22 ++++++-----
>>>>>>  app/test-mldev/ml_options.c                   | 18 ++++-----
>>>>>>  drivers/bus/fslmc/fslmc_bus.c                 |  5 ++-
>>>>>>  drivers/bus/fslmc/portal/dpaa2_hw_dpio.c      |  4 +-
>>>>>>  drivers/common/cnxk/cnxk_telemetry_nix.c      | 12 +++---
>>>>>>  drivers/event/cnxk/cnxk_eventdev.c            | 10 +++--
>>>>>>  drivers/event/cnxk/cnxk_tim_evdev.c           | 11 +++---
>>>>>>  drivers/net/ark/ark_pktchkr.c                 | 10 ++---
>>>>>>  drivers/net/ark/ark_pktgen.c                  | 10 ++---
>>>>>>  drivers/raw/cnxk_gpio/cnxk_gpio.c             |  6 +--
>>>>>>  examples/l2fwd-crypto/main.c                  |  6 +--
>>>>>>  examples/vhost/main.c                         |  3 +-
>>>>>>  lib/dmadev/rte_dmadev.c                       |  4 +-
>>>>>>  lib/eal/common/eal_common_memory.c            |  8 ++--
>>>>>>  lib/ethdev/rte_ethdev_telemetry.c             |  6 ++-
>>>>>>  lib/eventdev/rte_event_eth_rx_adapter.c       | 38 +++++++++----------
>>>>>>  lib/eventdev/rte_eventdev.c                   | 18 ++++-----
>>>>>>  lib/security/rte_security.c                   |  3 +-
>>>>>>  lib/telemetry/telemetry.c                     |  5 ++-
>>>>>>  27 files changed, 169 insertions(+), 140 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.30.0
>>>>> .
> .
>