* [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check @ 2019-07-23 11:25 Jiri Pirko 2019-07-23 11:25 ` [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch Jiri Pirko ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Jiri Pirko @ 2019-07-23 11:25 UTC (permalink / raw) To: netdev; +Cc: sthemmin, dsahern, alexanderk, mlxsw From: Jiri Pirko <jiri@mellanox.com> One cannot depend on *argv being null in case of no arg is left on the command line. For example in batch mode, this is not always true. Check argc instead to prevent crash. Reported-by: Alex Kushnarov <alexanderk@mellanox.com> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") Signed-off-by: Jiri Pirko <jiri@mellanox.com> --- tc/m_action.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tc/m_action.c b/tc/m_action.c index ab6bc0ad28ff..0f9c3a27795d 100644 --- a/tc/m_action.c +++ b/tc/m_action.c @@ -222,7 +222,7 @@ done0: goto bad_val; } - if (*argv && strcmp(*argv, "cookie") == 0) { + if (argc && strcmp(*argv, "cookie") == 0) { size_t slen; NEXT_ARG(); -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch 2019-07-23 11:25 [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Jiri Pirko @ 2019-07-23 11:25 ` Jiri Pirko 2019-07-26 21:35 ` Stephen Hemminger 2019-07-23 17:47 ` [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Stephen Hemminger ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Jiri Pirko @ 2019-07-23 11:25 UTC (permalink / raw) To: netdev; +Cc: sthemmin, dsahern, alexanderk, mlxsw From: Jiri Pirko <jiri@mellanox.com> When getcmdline fails, there is no valid string in line_next. So change the flow and don't process it. Alongside with that, free the previous line buffer and prevent memory leak. Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions") Signed-off-by: Jiri Pirko <jiri@mellanox.com> --- tc/tc.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/tc/tc.c b/tc/tc.c index 64e342dd85bf..8abc82aedcf8 100644 --- a/tc/tc.c +++ b/tc/tc.c @@ -325,11 +325,10 @@ static int batch(const char *name) { struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL; char *largv[100], *largv_next[100]; - char *line, *line_next = NULL; + char *line = NULL, *line_next = NULL; bool bs_enabled = false; bool lastline = false; int largc, largc_next; - bool bs_enabled_saved; bool bs_enabled_next; int batchsize = 0; size_t len = 0; @@ -360,11 +359,13 @@ static int batch(const char *name) largc = makeargs(line, largv, 100); bs_enabled = batchsize_enabled(largc, largv); do { - if (getcmdline(&line_next, &len, stdin) == -1) + if (getcmdline(&line_next, &len, stdin) == -1) { lastline = true; - - largc_next = makeargs(line_next, largv_next, 100); - bs_enabled_next = batchsize_enabled(largc_next, largv_next); + bs_enabled_next = false; + } else { + largc_next = makeargs(line_next, largv_next, 100); + bs_enabled_next = batchsize_enabled(largc_next, largv_next); + } if (bs_enabled) { struct batch_buf *buf; @@ -389,17 +390,8 @@ static int batch(const char *name) else send = true; - line = line_next; - line_next = NULL; - len = 0; - bs_enabled_saved = bs_enabled; - bs_enabled = bs_enabled_next; - - if (largc == 0) { - largc = largc_next; - memcpy(largv, largv_next, largc * sizeof(char *)); - continue; /* blank line */ - } + if (largc == 0) + goto to_next_line; /* blank line */ err = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf, tail == NULL ? 0 : sizeof(tail->buf)); @@ -411,10 +403,8 @@ static int batch(const char *name) if (!force) break; } - largc = largc_next; - memcpy(largv, largv_next, largc * sizeof(char *)); - if (send && bs_enabled_saved) { + if (send && bs_enabled) { struct iovec *iov, *iovs; struct batch_buf *buf; struct nlmsghdr *n; @@ -438,6 +428,18 @@ static int batch(const char *name) } batchsize = 0; } + +to_next_line: + if (lastline) + continue; + free(line); + line = line_next; + line_next = NULL; + len = 0; + bs_enabled = bs_enabled_next; + largc = largc_next; + memcpy(largv, largv_next, largc * sizeof(char *)); + } while (!lastline); free_batch_bufs(&buf_pool); -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch 2019-07-23 11:25 ` [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch Jiri Pirko @ 2019-07-26 21:35 ` Stephen Hemminger 0 siblings, 0 replies; 10+ messages in thread From: Stephen Hemminger @ 2019-07-26 21:35 UTC (permalink / raw) To: Jiri Pirko; +Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw On Tue, 23 Jul 2019 13:25:38 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > From: Jiri Pirko <jiri@mellanox.com> > > When getcmdline fails, there is no valid string in line_next. > So change the flow and don't process it. Alongside with that, > free the previous line buffer and prevent memory leak. > > Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions") > Signed-off-by: Jiri Pirko <jiri@mellanox.com> This is not sufficient to avoid valgrind detected uninitialized memory. The following changes to your patch (#2 alone) is enough to fix the issue. The logic here is still a mess and needs to be cleaned up to avoid future related bugs. From bbcc22899566556ced9692e4811aea2a38817834 Mon Sep 17 00:00:00 2001 From: Jiri Pirko <jiri@mellanox.com> Date: Tue, 23 Jul 2019 13:25:38 +0200 Subject: [PATCH] tc: batch: fix line/line_next processing in batch When getcmdline fails, there is no valid string in line_next. So change the flow and don't process it. Alongside with that, free the previous line buffer and prevent memory leak. Also, avoid passing uninitialized memory to filters. Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions") Signed-off-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- tc/tc.c | 84 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/tc/tc.c b/tc/tc.c index 64e342dd85bf..95e5481955ad 100644 --- a/tc/tc.c +++ b/tc/tc.c @@ -325,11 +325,10 @@ static int batch(const char *name) { struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL; char *largv[100], *largv_next[100]; - char *line, *line_next = NULL; + char *line = NULL, *line_next = NULL; bool bs_enabled = false; bool lastline = false; int largc, largc_next; - bool bs_enabled_saved; bool bs_enabled_next; int batchsize = 0; size_t len = 0; @@ -360,47 +359,40 @@ static int batch(const char *name) largc = makeargs(line, largv, 100); bs_enabled = batchsize_enabled(largc, largv); do { - if (getcmdline(&line_next, &len, stdin) == -1) + if (getcmdline(&line_next, &len, stdin) == -1) { lastline = true; - - largc_next = makeargs(line_next, largv_next, 100); - bs_enabled_next = batchsize_enabled(largc_next, largv_next); - if (bs_enabled) { - struct batch_buf *buf; - - buf = get_batch_buf(&buf_pool, &head, &tail); - if (!buf) { - fprintf(stderr, - "failed to allocate batch_buf\n"); - return -1; - } - ++batchsize; - } - - /* - * In batch mode, if we haven't accumulated enough commands - * and this is not the last command and this command & next - * command both support the batchsize feature, don't send the - * message immediately. - */ - if (!lastline && bs_enabled && bs_enabled_next - && batchsize != MSG_IOV_MAX) - send = false; - else send = true; + } else { + largc_next = makeargs(line_next, largv_next, 100); + bs_enabled_next = batchsize_enabled(largc_next, largv_next); + if (bs_enabled) { + struct batch_buf *buf; + + buf = get_batch_buf(&buf_pool, &head, &tail); + if (!buf) { + fprintf(stderr, + "failed to allocate batch_buf\n"); + return -1; + } + ++batchsize; + } - line = line_next; - line_next = NULL; - len = 0; - bs_enabled_saved = bs_enabled; - bs_enabled = bs_enabled_next; - - if (largc == 0) { - largc = largc_next; - memcpy(largv, largv_next, largc * sizeof(char *)); - continue; /* blank line */ + /* + * In batch mode, if we haven't accumulated enough + * commands and this is not the last command and this + * command & next command both support the batchsize + * feature, don't send the message immediately. + */ + if (bs_enabled && bs_enabled_next + && batchsize != MSG_IOV_MAX) + send = false; + else + send = true; } + if (largc == 0) + goto to_next_line; /* blank line */ + err = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf, tail == NULL ? 0 : sizeof(tail->buf)); fflush(stdout); @@ -411,10 +403,8 @@ static int batch(const char *name) if (!force) break; } - largc = largc_next; - memcpy(largv, largv_next, largc * sizeof(char *)); - if (send && bs_enabled_saved) { + if (send && bs_enabled) { struct iovec *iov, *iovs; struct batch_buf *buf; struct nlmsghdr *n; @@ -438,6 +428,18 @@ static int batch(const char *name) } batchsize = 0; } + +to_next_line: + if (lastline) + continue; + free(line); + line = line_next; + line_next = NULL; + len = 0; + bs_enabled = bs_enabled_next; + largc = largc_next; + memcpy(largv, largv_next, largc * sizeof(char *)); + } while (!lastline); free_batch_bufs(&buf_pool); -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-23 11:25 [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Jiri Pirko 2019-07-23 11:25 ` [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch Jiri Pirko @ 2019-07-23 17:47 ` Stephen Hemminger 2019-07-23 17:54 ` Stephen Hemminger 2019-07-26 19:47 ` Stephen Hemminger 3 siblings, 0 replies; 10+ messages in thread From: Stephen Hemminger @ 2019-07-23 17:47 UTC (permalink / raw) To: Jiri Pirko; +Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw On Tue, 23 Jul 2019 13:25:37 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > From: Jiri Pirko <jiri@mellanox.com> > > One cannot depend on *argv being null in case of no arg is left on the > command line. For example in batch mode, this is not always true. Check > argc instead to prevent crash. > > Reported-by: Alex Kushnarov <alexanderk@mellanox.com> > Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > tc/m_action.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tc/m_action.c b/tc/m_action.c > index ab6bc0ad28ff..0f9c3a27795d 100644 > --- a/tc/m_action.c > +++ b/tc/m_action.c > @@ -222,7 +222,7 @@ done0: > goto bad_val; > } > > - if (*argv && strcmp(*argv, "cookie") == 0) { > + if (argc && strcmp(*argv, "cookie") == 0) { > size_t slen; > > NEXT_ARG(); Ok, but we should also fix batch mode to null last argv ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-23 11:25 [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Jiri Pirko 2019-07-23 11:25 ` [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch Jiri Pirko 2019-07-23 17:47 ` [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Stephen Hemminger @ 2019-07-23 17:54 ` Stephen Hemminger 2019-07-23 19:36 ` Jiri Pirko 2019-07-24 9:07 ` David Laight 2019-07-26 19:47 ` Stephen Hemminger 3 siblings, 2 replies; 10+ messages in thread From: Stephen Hemminger @ 2019-07-23 17:54 UTC (permalink / raw) To: Jiri Pirko; +Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw On Tue, 23 Jul 2019 13:25:37 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > From: Jiri Pirko <jiri@mellanox.com> > > One cannot depend on *argv being null in case of no arg is left on the > command line. For example in batch mode, this is not always true. Check > argc instead to prevent crash. > > Reported-by: Alex Kushnarov <alexanderk@mellanox.com> > Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") > Signed-off-by: Jiri Pirko <jiri@mellanox.com> Actually makeargs does NULL terminate the last arg so what input to batchmode is breaking this? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-23 17:54 ` Stephen Hemminger @ 2019-07-23 19:36 ` Jiri Pirko 2019-07-26 19:00 ` Stephen Hemminger 2019-07-24 9:07 ` David Laight 1 sibling, 1 reply; 10+ messages in thread From: Jiri Pirko @ 2019-07-23 19:36 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw Tue, Jul 23, 2019 at 07:54:01PM CEST, stephen@networkplumber.org wrote: >On Tue, 23 Jul 2019 13:25:37 +0200 >Jiri Pirko <jiri@resnulli.us> wrote: > >> From: Jiri Pirko <jiri@mellanox.com> >> >> One cannot depend on *argv being null in case of no arg is left on the >> command line. For example in batch mode, this is not always true. Check >> argc instead to prevent crash. >> >> Reported-by: Alex Kushnarov <alexanderk@mellanox.com> >> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> > >Actually makeargs does NULL terminate the last arg so what input >to batchmode is breaking this? Interesting, there must be another but out there then. My input is: filter add dev testdummy parent ffff: protocol all prio 11000 flower action drop filter add dev testdummy parent ffff: protocol ipv4 prio 1 flower dst_mac 11:22:33:44:55:66 action drop ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-23 19:36 ` Jiri Pirko @ 2019-07-26 19:00 ` Stephen Hemminger 0 siblings, 0 replies; 10+ messages in thread From: Stephen Hemminger @ 2019-07-26 19:00 UTC (permalink / raw) To: Jiri Pirko; +Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw On Tue, 23 Jul 2019 21:36:00 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > Tue, Jul 23, 2019 at 07:54:01PM CEST, stephen@networkplumber.org wrote: > >On Tue, 23 Jul 2019 13:25:37 +0200 > >Jiri Pirko <jiri@resnulli.us> wrote: > > > >> From: Jiri Pirko <jiri@mellanox.com> > >> > >> One cannot depend on *argv being null in case of no arg is left on the > >> command line. For example in batch mode, this is not always true. Check > >> argc instead to prevent crash. > >> > >> Reported-by: Alex Kushnarov <alexanderk@mellanox.com> > >> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") > >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> > > > >Actually makeargs does NULL terminate the last arg so what input > >to batchmode is breaking this? > > Interesting, there must be another but out there then. > > My input is: > filter add dev testdummy parent ffff: protocol all prio 11000 flower action drop > filter add dev testdummy parent ffff: protocol ipv4 prio 1 flower dst_mac 11:22:33:44:55:66 action drop This maybe related. Looks like the batchsize patches had issues. # valgrind ./tc/tc -batch filter.bat ==27348== Memcheck, a memory error detector ==27348== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==27348== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info ==27348== Command: ./tc/tc -batch filter.bat ==27348== ==27348== Conditional jump or move depends on uninitialised value(s) ==27348== at 0x4EE9C0C: getdelim (iogetdelim.c:59) ==27348== by 0x152A37: getline (stdio.h:120) ==27348== by 0x152A37: getcmdline (utils.c:1311) ==27348== by 0x115543: batch (tc.c:358) ==27348== by 0x4E9D09A: (below main) (libc-start.c:308) ==27348== ==27348== Conditional jump or move depends on uninitialised value(s) ==27348== at 0x152BE4: makeargs (utils.c:1359) ==27348== by 0x115614: batch (tc.c:366) ==27348== by 0x4E9D09A: (below main) (libc-start.c:308) ==27348== ==27348== Conditional jump or move depends on uninitialised value(s) ==27348== at 0x11EBFD: parse_action (m_action.c:225) ==27348== by 0x13633E: flower_parse_opt (f_flower.c:1285) ==27348== by 0x1190EB: tc_filter_modify (tc_filter.c:217) ==27348== by 0x115674: batch (tc.c:404) ==27348== by 0x4E9D09A: (below main) (libc-start.c:308) ==27348== ==27348== Use of uninitialised value of size 8 ==27348== at 0x11EC0B: parse_action (m_action.c:225) ==27348== by 0x13633E: flower_parse_opt (f_flower.c:1285) ==27348== by 0x1190EB: tc_filter_modify (tc_filter.c:217) ==27348== by 0x115674: batch (tc.c:404) ==27348== by 0x4E9D09A: (below main) (libc-start.c:308) ==27348== Error: Parent Qdisc doesn't exists. Error: Parent Qdisc doesn't exists. Command failed filter.bat:1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-23 17:54 ` Stephen Hemminger 2019-07-23 19:36 ` Jiri Pirko @ 2019-07-24 9:07 ` David Laight 1 sibling, 0 replies; 10+ messages in thread From: David Laight @ 2019-07-24 9:07 UTC (permalink / raw) To: 'Stephen Hemminger', Jiri Pirko Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw From: Stephen Hemminger > Sent: 23 July 2019 18:54 > > On Tue, 23 Jul 2019 13:25:37 +0200 > Jiri Pirko <jiri@resnulli.us> wrote: > > > From: Jiri Pirko <jiri@mellanox.com> > > > > One cannot depend on *argv being null in case of no arg is left on the > > command line. For example in batch mode, this is not always true. Check > > argc instead to prevent crash. Hmmm... expecting the increments of argv and decrements of argc to match it probably wishful thinking.... A lot of parsers don't even look at argc. > Actually makeargs does NULL terminate the last arg so what input > to batchmode is breaking this? The 'usual' problem is an extra increment of argv because the last entry was something that 'eats' two or more entries. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-23 11:25 [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Jiri Pirko ` (2 preceding siblings ...) 2019-07-23 17:54 ` Stephen Hemminger @ 2019-07-26 19:47 ` Stephen Hemminger 2019-07-27 8:36 ` Jiri Pirko 3 siblings, 1 reply; 10+ messages in thread From: Stephen Hemminger @ 2019-07-26 19:47 UTC (permalink / raw) To: Jiri Pirko, chrims; +Cc: netdev, sthemmin, dsahern, alexanderk, mlxsw On Tue, 23 Jul 2019 13:25:37 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > From: Jiri Pirko <jiri@mellanox.com> > > One cannot depend on *argv being null in case of no arg is left on the > command line. For example in batch mode, this is not always true. Check > argc instead to prevent crash. > > Reported-by: Alex Kushnarov <alexanderk@mellanox.com> > Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > tc/m_action.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tc/m_action.c b/tc/m_action.c > index ab6bc0ad28ff..0f9c3a27795d 100644 > --- a/tc/m_action.c > +++ b/tc/m_action.c > @@ -222,7 +222,7 @@ done0: > goto bad_val; > } > > - if (*argv && strcmp(*argv, "cookie") == 0) { > + if (argc && strcmp(*argv, "cookie") == 0) { > size_t slen; > > NEXT_ARG(); The logic here is broken at end of file. do { if (getcmdline(&line_next, &len, stdin) == -1) lastline = true; largc_next = makeargs(line_next, largv_next, 100); bs_enabled_next = batchsize_enabled(largc_next, largv_next); if (bs_enabled) { struct batch_ getcmdline() will return -1 at end of file. The code will call make_args on an uninitialized pointer. I see lots of other unnecessary complexity in the whole batch logic. It needs to be rewritten. Rather than me fixing the code, I am probably going to revert. commit 485d0c6001c4aa134b99c86913d6a7089b7b2ab0 Author: Chris Mi <chrism@mellanox.com> Date: Fri Jan 12 14:13:16 2018 +0900 tc: Add batchsize feature for filter and actions ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check 2019-07-26 19:47 ` Stephen Hemminger @ 2019-07-27 8:36 ` Jiri Pirko 0 siblings, 0 replies; 10+ messages in thread From: Jiri Pirko @ 2019-07-27 8:36 UTC (permalink / raw) To: Stephen Hemminger; +Cc: chrims, netdev, sthemmin, dsahern, alexanderk, mlxsw Fri, Jul 26, 2019 at 09:47:07PM CEST, stephen@networkplumber.org wrote: >On Tue, 23 Jul 2019 13:25:37 +0200 >Jiri Pirko <jiri@resnulli.us> wrote: > >> From: Jiri Pirko <jiri@mellanox.com> >> >> One cannot depend on *argv being null in case of no arg is left on the >> command line. For example in batch mode, this is not always true. Check >> argc instead to prevent crash. >> >> Reported-by: Alex Kushnarov <alexanderk@mellanox.com> >> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> tc/m_action.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tc/m_action.c b/tc/m_action.c >> index ab6bc0ad28ff..0f9c3a27795d 100644 >> --- a/tc/m_action.c >> +++ b/tc/m_action.c >> @@ -222,7 +222,7 @@ done0: >> goto bad_val; >> } >> >> - if (*argv && strcmp(*argv, "cookie") == 0) { >> + if (argc && strcmp(*argv, "cookie") == 0) { >> size_t slen; >> >> NEXT_ARG(); > > >The logic here is broken at end of file. > > do { > if (getcmdline(&line_next, &len, stdin) == -1) > lastline = true; > > largc_next = makeargs(line_next, largv_next, 100); > bs_enabled_next = batchsize_enabled(largc_next, largv_next); > if (bs_enabled) { > struct batch_ > > >getcmdline() will return -1 at end of file. >The code will call make_args on an uninitialized pointer. > >I see lots of other unnecessary complexity in the whole batch logic. >It needs to be rewritten. > >Rather than me fixing the code, I am probably going to revert. I agree. This is a mess :( > >commit 485d0c6001c4aa134b99c86913d6a7089b7b2ab0 >Author: Chris Mi <chrism@mellanox.com> >Date: Fri Jan 12 14:13:16 2018 +0900 > > tc: Add batchsize feature for filter and actions ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-27 8:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-23 11:25 [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Jiri Pirko 2019-07-23 11:25 ` [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch Jiri Pirko 2019-07-26 21:35 ` Stephen Hemminger 2019-07-23 17:47 ` [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check Stephen Hemminger 2019-07-23 17:54 ` Stephen Hemminger 2019-07-23 19:36 ` Jiri Pirko 2019-07-26 19:00 ` Stephen Hemminger 2019-07-24 9:07 ` David Laight 2019-07-26 19:47 ` Stephen Hemminger 2019-07-27 8:36 ` Jiri Pirko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).