Re: [PATCH 05/10] cmdline: fix port list parsing

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jun 23, 2025 at 11:40:15AM +0200, David Marchand wrote:
> On Fri, Jun 20, 2025 at 11:59 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Thu, Jun 19, 2025 at 09:10:31AM +0200, David Marchand wrote:
> > > Doing arithmetics with the NULL pointer is undefined.
> > >
> > > Caught by UBSan:
> > >
> > > ../lib/cmdline/cmdline_parse_portlist.c:40:19: runtime error:
> > >       applying non-zero offset 1 to null pointer
> > > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> > >       ../lib/cmdline/cmdline_parse_portlist.c:40:19 in
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > >  lib/cmdline/cmdline_parse_portlist.c | 13 +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/cmdline/cmdline_parse_portlist.c b/lib/cmdline/cmdline_parse_portlist.c
> > > index ef6ce223b5..0c07cc02b5 100644
> > > --- a/lib/cmdline/cmdline_parse_portlist.c
> > > +++ b/lib/cmdline/cmdline_parse_portlist.c
> > > @@ -4,6 +4,7 @@
> > >   * All rights reserved.
> > >   */
> > >
> > > +#include <stdbool.h>
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > @@ -37,10 +38,11 @@ parse_ports(cmdline_portlist_t *pl, const char *str)
> > >       const char *first, *last;
> > >       char *end;
> > >
> > > -     for (first = str, last = first;
> > > -         first != NULL && last != NULL;
> > > -         first = last + 1) {
> >
> > Maybe I'm a little slow this morning, but I can't see how this is actually
> > a problem. By my understanding, the check for "first != NULL && last !=
> > NULL" happens before any increment of "first = last + 1", meaning we are
> > guaranteed that the last is never null when we increment it.
> 
> Well, not sure I follow, but the problem is not at the first
> iteration, if this is what you mean.
> 
> On the last iteration of the parsing, there is no , left in the string
> that is parsed so last = strchr(first, ',') makes last == NULL.
> Then the first variable is set to last + 1 *before* evaluating the end
> condition.
> 
> I removed this patch of the series, rerun the test and I see:
> 
> 9/75 DPDK:fast-tests / cmdline_autotest               OK              0.22s
> 09:20:08 DPDK_TEST=cmdline_autotest MALLOC_PERTURB_=169
> /home/runner/work/dpdk/dpdk/build/app/dpdk-test --no-huge -m 2048 -d
> /home/runner/work/dpdk/dpdk/build/drivers
> ----------------------------------- output -----------------------------------
> stdout:
> RTE>>cmdline_autotest
> Testind parsing ethernet addresses...
> Testind parsing port lists...
> Testind parsing numbers...
> Testing parsing IP addresses...
> Testing parsing strings...
> Testing circular buffer...
> Testing library functions...
> Test OK
> RTE>>
> stderr:
> EAL: Detected CPU lcores: 4
> EAL: Detected NUMA nodes: 1
> EAL: Detected shared linkage of DPDK
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'VA'
> APP: HPET is not enabled, using TSC as default timer
> ../lib/cmdline/cmdline_parse_portlist.c:44:19: runtime error: applying
> non-zero offset 1 to null pointer
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> ../lib/cmdline/cmdline_parse_portlist.c:44:19 in
> ------------------------------------------------------------------------------
> 
> 
Thanks for the explanation. I was indeed thinking the issue was on the
first iteration only.

With the change to fix this, we can actually make last a local var within
the loop itself. Also, by using a while rather than do-while we can remove
the initial check for str = NULL. Here's an alternate fix that is very
slightly shorter, and limits the scope of "last":

diff --git a/lib/cmdline/cmdline_parse_portlist.c b/lib/cmdline/cmdline_parse_portlist.c
index ebe2a961bb..c65f3b704e 100644
--- a/lib/cmdline/cmdline_parse_portlist.c
+++ b/lib/cmdline/cmdline_parse_portlist.c
@@ -34,14 +34,11 @@ static int
 parse_ports(cmdline_portlist_t *pl, const char *str)
 {
        size_t ps, pe;
-       const char *first, *last;
+       const char *first = str;
        char *end;
 
-       for (first = str, last = first;
-           first != NULL && last != NULL;
-           first = last + 1) {
-
-               last = strchr(first, ',');
+       while (first != NULL) {
+               const char *last = strchr(first, ',');
 
                errno = 0;
                ps = strtoul(first, &end, 10);
@@ -65,6 +62,8 @@ parse_ports(cmdline_portlist_t *pl, const char *str)
                        return -1;
 
                parse_set_list(pl, ps, pe);
+
+               first = (last == NULL ? NULL : last + 1);
        }
 
        return 0;