| Message ID | 20190723112538.10977-1-jiri@resnulli.us |
|---|---|
| State | Rejected |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2,1/2] tc: action: fix crash caused by incorrect *argv check | expand |
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
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?
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
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)
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
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
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
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();