test/eal: fix incorrect array sizes

  Following removal of -c and -n options, the array should have been
shrunk to avoid launch_proc to access unitialised strings.

Fixes: b4dbacdb1ad7 ("test/eal: set core mask/list config only in dedicated test")
Fixes: 501fa9a40298 ("test/eal: set memory channel config only in dedicated test")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_eal_flags.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

On Wed, Jul 31, 2019 at 2:56 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Following removal of -c and -n options, the array should have been
> shrunk to avoid launch_proc to access unitialised strings.
>
> Fixes: b4dbacdb1ad7 ("test/eal: set core mask/list config only in dedicated test")
> Fixes: 501fa9a40298 ("test/eal: set memory channel config only in dedicated test")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Good patch, but is it just me or are you off by one in some places? see inline
> ---
>  app/test/test_eal_flags.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 827ea88..5b2c0f5 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -244,7 +244,7 @@ test_whitelist_flag(void)
>         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
>  #endif
>
> -       const char *wlinval[][11] = {
> +       const char *wlinval[][7] = {
Shoudln't this be wlinval[][6]?
>                 {prgname, prefix, mp_flag,
>                                 pci_whitelist, "error", "", ""},
>                 {prgname, prefix, mp_flag,
> @@ -310,7 +310,7 @@ test_invalid_b_flag(void)
>         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
>  #endif
>
> -       const char *blinval[][9] = {
> +       const char *blinval[][5] = {
blinval[][6]?
>                 {prgname, prefix, mp_flag, "-b", "error"},
>                 {prgname, prefix, mp_flag, "-b", "0:0:0"},
>                 {prgname, prefix, mp_flag, "-b", "0:error:0.1"},
> @@ -414,7 +414,7 @@ test_invalid_r_flag(void)
>         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
>  #endif
>
> -       const char *rinval[][9] = {
> +       const char *rinval[][5] = {
rinval[][4]?
>                         {prgname, prefix, mp_flag, "-r", "error"},
>                         {prgname, prefix, mp_flag, "-r", "0"},
>                         {prgname, prefix, mp_flag, "-r", "-1"},
> --
> 1.8.3.1
>
  

On Wed, Jul 31, 2019 at 9:57 PM Michael Santana Francisco
<msantana@redhat.com> wrote:
>
> On Wed, Jul 31, 2019 at 2:56 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > Following removal of -c and -n options, the array should have been
> > shrunk to avoid launch_proc to access unitialised strings.
> >
> > Fixes: b4dbacdb1ad7 ("test/eal: set core mask/list config only in dedicated test")
> > Fixes: 501fa9a40298 ("test/eal: set memory channel config only in dedicated test")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Good patch, but is it just me or are you off by one in some places? see inline

The "funny" thing is that it went unnoticed, because the strdup
happens after the test binary forked a child to start a dpdk process.
This child segfaults, but the test code running in the parent just
checks that starting this child failed (since it passed incorrect eal
arguments).


> > ---
> >  app/test/test_eal_flags.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> > index 827ea88..5b2c0f5 100644
> > --- a/app/test/test_eal_flags.c
> > +++ b/app/test/test_eal_flags.c
> > @@ -244,7 +244,7 @@ test_whitelist_flag(void)
> >         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> >  #endif
> >
> > -       const char *wlinval[][11] = {
> > +       const char *wlinval[][7] = {
> Shoudln't this be wlinval[][6]?

I can see 7 elements in this array.

> >                 {prgname, prefix, mp_flag,
> >                                 pci_whitelist, "error", "", ""},
> >                 {prgname, prefix, mp_flag,
> > @@ -310,7 +310,7 @@ test_invalid_b_flag(void)
> >         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> >  #endif
> >
> > -       const char *blinval[][9] = {
> > +       const char *blinval[][5] = {
> blinval[][6]?

5 elements.

> >                 {prgname, prefix, mp_flag, "-b", "error"},
> >                 {prgname, prefix, mp_flag, "-b", "0:0:0"},
> >                 {prgname, prefix, mp_flag, "-b", "0:error:0.1"},
> > @@ -414,7 +414,7 @@ test_invalid_r_flag(void)
> >         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> >  #endif
> >
> > -       const char *rinval[][9] = {
> > +       const char *rinval[][5] = {
> rinval[][4]?

Idem.

> >                         {prgname, prefix, mp_flag, "-r", "error"},
> >                         {prgname, prefix, mp_flag, "-r", "0"},
> >                         {prgname, prefix, mp_flag, "-r", "-1"},
> > --
> > 1.8.3.1
> >
  

On Thu, Aug 1, 2019 at 3:25 AM David Marchand <david.marchand@redhat.com> wrote:
>
> On Wed, Jul 31, 2019 at 9:57 PM Michael Santana Francisco
> <msantana@redhat.com> wrote:
> >
> > On Wed, Jul 31, 2019 at 2:56 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > Following removal of -c and -n options, the array should have been
> > > shrunk to avoid launch_proc to access unitialised strings.
> > >
> > > Fixes: b4dbacdb1ad7 ("test/eal: set core mask/list config only in dedicated test")
> > > Fixes: 501fa9a40298 ("test/eal: set memory channel config only in dedicated test")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Good patch, but is it just me or are you off by one in some places? see inline
>
> The "funny" thing is that it went unnoticed, because the strdup
> happens after the test binary forked a child to start a dpdk process.
> This child segfaults, but the test code running in the parent just
> checks that starting this child failed (since it passed incorrect eal
> arguments).
>
>
> > > ---
> > >  app/test/test_eal_flags.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> > > index 827ea88..5b2c0f5 100644
> > > --- a/app/test/test_eal_flags.c
> > > +++ b/app/test/test_eal_flags.c
> > > @@ -244,7 +244,7 @@ test_whitelist_flag(void)
> > >         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> > >  #endif
> > >
> > > -       const char *wlinval[][11] = {
> > > +       const char *wlinval[][7] = {
> > Shoudln't this be wlinval[][6]?
>
> I can see 7 elements in this array.
My apologies, your patch was right from the start . I was counting the
wrong thing
Acked-by: Michael Santana <msantana@redhat.com>
>
> > >                 {prgname, prefix, mp_flag,
> > >                                 pci_whitelist, "error", "", ""},
> > >                 {prgname, prefix, mp_flag,
> > > @@ -310,7 +310,7 @@ test_invalid_b_flag(void)
> > >         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> > >  #endif
> > >
> > > -       const char *blinval[][9] = {
> > > +       const char *blinval[][5] = {
> > blinval[][6]?
>
> 5 elements.
>
> > >                 {prgname, prefix, mp_flag, "-b", "error"},
> > >                 {prgname, prefix, mp_flag, "-b", "0:0:0"},
> > >                 {prgname, prefix, mp_flag, "-b", "0:error:0.1"},
> > > @@ -414,7 +414,7 @@ test_invalid_r_flag(void)
> > >         snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> > >  #endif
> > >
> > > -       const char *rinval[][9] = {
> > > +       const char *rinval[][5] = {
> > rinval[][4]?
>
> Idem.
>
> > >                         {prgname, prefix, mp_flag, "-r", "error"},
> > >                         {prgname, prefix, mp_flag, "-r", "0"},
> > >                         {prgname, prefix, mp_flag, "-r", "-1"},
> > > --
> > > 1.8.3.1
> > >
>
>
> --
> David Marchand
  

> > > > Following removal of -c and -n options, the array should have been
> > > > shrunk to avoid launch_proc to access unitialised strings.
> > > >
> > > > Fixes: b4dbacdb1ad7 ("test/eal: set core mask/list config only in dedicated test")
> > > > Fixes: 501fa9a40298 ("test/eal: set memory channel config only in dedicated test")
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Michael Santana <msantana@redhat.com>

Applied, thanks.

About the title: "incorrect" after "fix" is always useless.
I replaced it with "command" to give a bit more context about
what these arrays are for.