| Message ID | 20231113104550.2138654-1-haijie1@huawei.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1C533430AB; Mon, 13 Nov 2023 11:50:52 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B9D20402E7; Mon, 13 Nov 2023 11:50:51 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id EFB404021F for <dev@dpdk.org>; Mon, 13 Nov 2023 11:50:49 +0100 (CET) Received: from kwepemd100004.china.huawei.com (unknown [172.30.72.56]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4STR2p6sGyz1P89y for <dev@dpdk.org>; Mon, 13 Nov 2023 18:47:30 +0800 (CST) Received: from localhost.localdomain (10.67.165.2) by kwepemd100004.china.huawei.com (7.221.188.31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.2.1258.23; Mon, 13 Nov 2023 18:50:47 +0800 From: Jie Hai <haijie1@huawei.com> To: <dev@dpdk.org> CC: <haijie1@huawei.com>, <lihuisong@huawei.com>, <fengchengwen@huawei.com> Subject: [PATCH 00/21] replace strtok with strtok_r Date: Mon, 13 Nov 2023 18:45:29 +0800 Message-ID: <20231113104550.2138654-1-haijie1@huawei.com> X-Mailer: git-send-email 2.30.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.67.165.2] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemd100004.china.huawei.com (7.221.188.31) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
| Series |
replace strtok with strtok_r
|
|
Message
Jie Hai
Nov. 13, 2023, 10:45 a.m. UTC
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. 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(-)
Comments
13/11/2023 11:45, Jie Hai: > 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. You should add a check in checkpatches.sh so we won't add more in future.
Hi Jie, Good fix There are two minor I think need to modify: 1. The [PATCH 13/21] lib/telemetry should be lib/security 2. All commits should add Cc because it's potential bug. The other LGTM, with above fixed Series-acked-by: Chengwen Feng <fengchengwen@huawei.com> Thanks Chengwen On 2023/11/13 18:45, 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. > > 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(-) >
On Mon, 13 Nov 2023 18:45:29 +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. > > The strtok() is non-reentrant, it is better to replace it with a > reentrant function. Lots of churn. And most of these places are never callable from multiple threads. It does indicate that some better arg handling is needed.
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! > > 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
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? 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 > .
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) 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 > >.
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. 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 > > >.
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. 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 > > > >.
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 >>>>> . > . >
> From: fengchengwen [mailto:fengchengwen@huawei.com] > Sent: Wednesday, 15 November 2023 04.03 > > 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. +1 to rte_strtok doing the reentrant dance for us. If we had such an rte_strtok(), we could also generally disallow the use of strtok(). > > > > > what do others think? > >
On Wed, 15 Nov 2023 12:27:37 +0100 Morten Brørup <mb@smartsharesystems.com> wrote: > > > 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. > > +1 to rte_strtok doing the reentrant dance for us. > > If we had such an rte_strtok(), we could also generally disallow the use of strtok(). Good idea, I like this. Would be good to have a version on Windows as well.
On 2023/11/15 23:08, Stephen Hemminger wrote: > On Wed, 15 Nov 2023 12:27:37 +0100 > Morten Brørup <mb@smartsharesystems.com> wrote: > >>>> 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. >> >> +1 to rte_strtok doing the reentrant dance for us. >> >> If we had such an rte_strtok(), we could also generally disallow the use of strtok(). > > Good idea, I like this. > Would be good to have a version on Windows as well. > > . Hi, all maintainers, Since the map of strtok_r to strtok_s with three patramaters has been done in "rte_os_shim.h", I just include this file. And the rte_strtok is defined as below. My question is whether the "strtok_s(s, stringlen, delim, save_ptr)" and "strtok_s(str, delim, saveptr)" are compatible for C11. And I check the glibc codes here, https://sourceware.org/git/glibc.git there is no definition of strtok_s, is strtok_s not in use? If so, should we delayed processing for the C11 standard strtok_s function? That is, delete the "#ifdef __STDC_LIB_EXT1__... #endif" part, but keep the extension of the four parameters for later use. Or keep the following change but never use stringlen as not null until the function can be used. What do you think? diff --git a/lib/eal/include/rte_string_fns.h b/lib/eal/include/rte_string_fns.h index bb43b2cba3eb..6746bfec384b 100644 --- a/lib/eal/include/rte_string_fns.h +++ b/lib/eal/include/rte_string_fns.h @@ -15,10 +15,13 @@ extern "C" { #endif +#define __STDC_WANT_LIB_EXT1__ 1 #include <stdio.h> #include <string.h> #include <rte_common.h> +#include <rte_compat.h> +#include <rte_os_shim.h> /** * Takes string "string" parameter and splits it at character "delim" @@ -115,6 +118,35 @@ rte_strlcat(char *dst, const char *src, size_t size) ssize_t rte_strscpy(char *dst, const char *src, size_t dsize); +/* + * Divide string s into tokens separated by characters in delim. + * Information passed between calls are stored in save_ptr. + * The size of the string to be separated is stored in *stringlen. + * + * @param s + * The string to be separated, it could be NULL. + * + * @param stringlen + * The pointor to the size of the string to be separated, it could be NULL. + * + * @param delim + * The characters on which the split of the data will be done. + * + * @param save_ptr + * The internal state of the string separated. + */ +static inline char * +rte_strtok(char *__rte_restrict s, size_t *__rte_restrict stringlen, + const char *__rte_restrict delim, + char **__rte_restrict save_ptr) +{ +#ifdef __STDC_LIB_EXT1__ + if (stringlen != NULL) + return strtok_s(s, stringlen, delim, save_ptr); +#endif + (void)stringlen; + return strtok_r(s, delim, save_ptr); +} + #ifdef __cplusplus } #endif Thanks, Jie Hai
On Tue, Nov 21, 2023 at 4:33 AM Jie Hai <haijie1@huawei.com> wrote: > > On 2023/11/15 23:08, Stephen Hemminger wrote: > > On Wed, 15 Nov 2023 12:27:37 +0100 > > Morten Brørup <mb@smartsharesystems.com> wrote: > > > >>>> 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. > >> > >> +1 to rte_strtok doing the reentrant dance for us. > >> > >> If we had such an rte_strtok(), we could also generally disallow the use of strtok(). > > > > Good idea, I like this. > > Would be good to have a version on Windows as well. > > > > . > Hi, all maintainers, > > Since the map of strtok_r to strtok_s with three patramaters has > been done in "rte_os_shim.h", I just include this file. > And the rte_strtok is defined as below. > My question is whether the > "strtok_s(s, stringlen, delim, save_ptr)" and > "strtok_s(str, delim, saveptr)" are compatible for C11. > > And I check the glibc codes here, > https://sourceware.org/git/glibc.git > there is no definition of strtok_s, is strtok_s not in use? > > If so, should we delayed processing for the C11 standard strtok_s > function? That is, delete the "#ifdef __STDC_LIB_EXT1__... > #endif" part, but keep the extension of the four parameters for later > use. Or keep the following change but never use stringlen as not null > until the function can be used. > > What do you think? Regardless of how it is done internally, I would not inline this helper. This will minimise headaches with checks against toolchain/compiler support in a DPDK header.