From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC83BCCF2DA for ; Mon, 19 Oct 2026 08:36:18 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8116B40608; Mon, 19 Oct 2026 10:36:17 +0200 (CEST) Received: from canpmsgout12.his.huawei.com (canpmsgout12.his.huawei.com [113.46.200.227]) by mails.dpdk.org (Postfix) with ESMTP id 3892540269; Mon, 19 Oct 2026 10:36:16 +0200 (CEST) dkim-signature: v=1; a=rsa-sha256; d=huawei.com; s=dkim; c=relaxed/relaxed; q=dns/txt; h=From; bh=jYs95Gyqib5SMufBqrMrNp5Tk313ok1H1tdVZhhmBMg=; b=EVEeP5vFusqqBkCZexys+HOOSaJEAMqCTx9R5FEwjyl3btpzygu73PmG+GIkFrf6EFYr9u6LE pcCF768N7bu9kiVVi9FMdPwukH4QtPvJrrzGDclcKikMSUgijgQ6pRGHVw3y6cYvOgiWH+Lv/1p 582dgkeJ+2qnAaYvyILqwDQ= Received: from mail.maildlp.com (unknown [172.19.163.200]) by canpmsgout12.his.huawei.com (SkyGuard) with ESMTPS id 4j7TR11V70znTxK; Mon, 19 Oct 2026 16:36:13 +0800 (CST) Received: from kwepemo500009.china.huawei.com (unknown [7.202.194.199]) by mail.maildlp.com (Postfix) with ESMTPS id B2C2E4056A; Mon, 19 Oct 2026 16:36:12 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by kwepemo500009.china.huawei.com (7.202.194.199) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.2562.45; Mon, 19 Oct 2026 16:36:12 +0800 Message-ID: <73b5e9cd-6789-422a-9679-88693c60352b@huawei.com> Date: Mon, 19 Oct 2026 16:36:12 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: |WARNING| argparse: support range step for corelist parsing To: , References: <<20261016093001.9067-1-fengchengwen@huawei.com> <20261016101228.123E46051830@postal.iol.unh.edu> Content-Language: en-US From: fengchengwen CC: "dev@dpdk.org" In-Reply-To: <20261016101228.123E46051830@postal.iol.unh.edu> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: kwepems200002.china.huawei.com (7.221.188.68) To kwepemo500009.china.huawei.com (7.202.194.199) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 10/16/2026 6:12 PM, dpdklab@iol.unh.edu wrote: > Test-Label: ai-code-review > Test-Status: WARNING > https://dpdk.org/patch/171477 > > _AI Code Review_ > > === Patch Review: series.patch (via Claude) === > Review date: 2026-10-16 > > # DPDK Patch Review > > ## Patch: argparse: support range step for corelist parsing > > ### ERRORS > > **Error 1: Integer overflow in step accumulation (correctness bug)** > > Line 629 in `lib/argparse/rte_argparse.c`: > ```c > for (; min <= max; min += step) > ``` > > When `step` is large and `min + step` exceeds `INT32_MAX`, the loop counter wraps around to a negative value, creating an infinite loop. The variables `min` and `max` are `int` (32-bit signed), and `step` is `int32_t`. If a malicious or mistaken user provides input like `"0-100:2147483647"`, the addition `min += step` will overflow after the first iteration. False positive, the: errno = 0; idx = strtol(value, &end, 10); last = end; if (errno || idx > UINT16_MAX) { ARGPARSE_LOG(ERR, "argument %s contains a numerical value out of range!", arg->name_long); return -EINVAL; } already treat such input as an error. > > **Suggested fix:** > Check that `step` does not cause overflow before the loop: > ```c > if (step > max - min) { > ARGPARSE_LOG(ERR, "argument %s step value too large for range!", > arg->name_long); > return -EINVAL; > } > for (; min <= max; min += step) > CPU_SET(min, cpuset); > ``` > > --- > > **Error 2: Step validation insufficient (correctness bug)** > > Lines 608-613 in `lib/argparse/rte_argparse.c`: > ```c > if (is_step) { > if (idx == 0) { > ARGPARSE_LOG(ERR, "argument %s step value must be positive!", > arg->name_long); > return -EINVAL; > } > step = idx; > } > ``` > > This only rejects zero but allows negative values. A negative step (e.g., `"1-5:-2"`) would pass this check because `idx` is `int64_t` and `-2 != 0`. The loop at line 629 would then decrement `min` instead of incrementing it, either becoming infinite or setting unintended cores. False positive, the: if (!isdigit(*value)) { ARGPARSE_LOG(ERR, "argument %s has an unexpected non digit character!", arg->name_long); return -EINVAL; } already treat -2 as an error. > > **Suggested fix:** > ```c > if (is_step) { > if (idx <= 0) { > ARGPARSE_LOG(ERR, "argument %s step value must be positive!", > arg->name_long); > return -EINVAL; > } > step = idx; > } > ``` > > --- > > **Error 3: Step value not clamped to 32-bit range before assignment (correctness bug)** > > Line 613 in `lib/argparse/rte_argparse.c`: > ```c > step = idx; > ``` > > `idx` is `int64_t` (from `rte_strtoint64_l()`), but `step` is `int32_t`. If the user provides `"0-10:4294967296"` (2^32), the assignment truncates the upper 32 bits, and `step` becomes 0, causing an infinite loop at line 629 or violating the earlier zero-check if the truncated value is negative. False positive, the: if (!isdigit(*value)) { ARGPARSE_LOG(ERR, "argument %s has an unexpected non digit character!", arg->name_long); return -EINVAL; } errno = 0; idx = strtol(value, &end, 10); last = end; if (errno || idx > UINT16_MAX) { ARGPARSE_LOG(ERR, "argument %s contains a numerical value out of range!", arg->name_long); return -EINVAL; } it mean min/max/step must be in [0, UINT16_MAX] after above verify > > **Suggested fix:** > Add a range check after the positive validation: > ```c > if (is_step) { > if (idx <= 0) { > ARGPARSE_LOG(ERR, "argument %s step value must be positive!", > arg->name_long); > return -EINVAL; > } > if (idx > INT32_MAX) { > ARGPARSE_LOG(ERR, "argument %s step value exceeds maximum!", > arg->name_long); > return -EINVAL; > } > step = (int32_t)idx; > } > ``` > > --- > > **Error 4: `max` and `min` signed/unsigned type confusion (correctness bug)** > > Lines 576-579 in `lib/argparse/rte_argparse.c`: > ```c > int32_t step = 1; > int32_t max = -1; > int min = -1; > ``` > > `min` is declared as `int` (typically 32-bit signed), while `max` is `int32_t` (explicitly 32-bit signed). This inconsistency is a latent bug: if `int` is 16-bit on some platform, `min` and `max` have different ranges. Modern DPDK targets assume 32-bit `int`, but the inconsistency is unnecessary and confusing. False positive, DPDK only support at least 32bit platform, the int is 32-bit at such platform. > > **Suggested fix:** > Declare `min` as `int32_t` for consistency: > ```c > int32_t step = 1; > int32_t max = -1; > int32_t min = -1; > ``` > > --- > > ### WARNINGS > > **Warning 1: Release notes missing for new user-visible feature** > > The patch adds a new user-facing syntax (`start-end:step`) to the `-l` corelist parser, which is a significant functional enhancement. The documentation in `doc/guides/prog_guide/argparse_lib.rst` is updated, but there is no corresponding entry in the release notes (`doc/guides/rel_notes/release_*.rst`) announcing this feature. > > **Suggested fix:** > Add a release notes entry under "New Features" in the current release notes file describing the new range step syntax for corelist parsing. OK, will done in v2 > > --- > > **Warning 2: Test coverage for boundary conditions incomplete** > > The test suite in `app/test/test_argparse.c` adds extensive tests for the step syntax, including invalid cases. However, it does not test: > - Maximum representable step values near `INT32_MAX` (boundary test) > - Step values that would cause integer overflow in the loop counter > - Step values larger than the range itself (e.g., `"0-5:10"`, which should set only core 0) > - Very large core numbers near `CPU_SETSIZE` (e.g., `"1000-1010:2"` if supported) OK, will add some of them > > **Suggested fix:** > Add tests for these boundary conditions to ensure the overflow checks (once added per Error 1) work correctly and that the implementation behaves as expected in edge cases. > > --- > > ### INFO > > None. > > --- > > ## Summary > > The patch introduces useful functionality but has four correctness bugs that must be fixed: > 1. Integer overflow in the step loop can cause infinite loops > 2. Negative step values are not rejected > 3. 64-to-32-bit step truncation is unchecked, allowing zero/negative truncated values > 4. Inconsistent `int` vs `int32_t` declaration for `min` and `max` > > Additionally, release notes are missing for this user-visible feature change, and test coverage could be improved for boundary conditions. > > --- > > Review-Result: ERROR