app/test-compress-perf: fix reliance on integer endianness
Checks
Commit Message
This patch fixes coverity issue:
Reliance on integer endianness (INCOMPATIBLE_CAST)
in parse_window_sz function.
Coverity issue: 328524
Fixes: e0b6287c035d ("app/compress-perf: add parser")
Cc: stable@dpdk.org
Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
---
app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Comments
HI Tomasz,
> -----Original Message-----
> From: Jozwiak, TomaszX
> Sent: Monday, May 20, 2019 2:26 PM
> To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak, TomaszX
> <tomaszx.jozwiak@intel.com>; shallyv@marvell.com; stable@dpdk.org
> Subject: [PATCH] app/test-compress-perf: fix reliance on integer endianness
>
> This patch fixes coverity issue:
> Reliance on integer endianness (INCOMPATIBLE_CAST)
> in parse_window_sz function.
>
> Coverity issue: 328524
> Fixes: e0b6287c035d ("app/compress-perf: add parser")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> ---
> app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-compress-perf/comp_perf_options_parse.c b/app/test-compress-
> perf/comp_perf_options_parse.c
> index 2fb6fb4..56ca580 100644
> --- a/app/test-compress-perf/comp_perf_options_parse.c
> +++ b/app/test-compress-perf/comp_perf_options_parse.c
> @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct comp_test_data *test_data, const char *arg)
> static int
> parse_window_sz(struct comp_test_data *test_data, const char *arg)
> {
> - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> + uint16_t tmp;
> + int ret = parse_uint16_t(&tmp, arg);
>
> if (ret) {
> RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> return -1;
> }
>
> + test_data->window_sz = (int)tmp;
> return 0;
> }
[Fiona] I expect this fixes this coverity issue - but will it result in another one?
window_sz on the xform is uint8_t - so this int will get truncated later, and there's no cast done at that point.
Would it be better to add a new parse_uint8_t fn and change test-data->window_sz to a unit8_t?
Or add that cast?
Hi Fiona,
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, May 20, 2019 4:06 PM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; dev@dpdk.org;
> shallyv@marvell.com; stable@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Trybula, ArturX
> <arturx.trybula@intel.com>
> Subject: RE: [PATCH] app/test-compress-perf: fix reliance on integer
> endianness
>
> HI Tomasz,
>
> > -----Original Message-----
> > From: Jozwiak, TomaszX
> > Sent: Monday, May 20, 2019 2:26 PM
> > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak,
> > TomaszX <tomaszx.jozwiak@intel.com>; shallyv@marvell.com;
> > stable@dpdk.org
> > Subject: [PATCH] app/test-compress-perf: fix reliance on integer
> > endianness
> >
> > This patch fixes coverity issue:
> > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> parse_window_sz
> > function.
> >
> > Coverity issue: 328524
> > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > ---
> > app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > b/app/test-compress- perf/comp_perf_options_parse.c index
> > 2fb6fb4..56ca580 100644
> > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> comp_test_data
> > *test_data, const char *arg) static int parse_window_sz(struct
> > comp_test_data *test_data, const char *arg) {
> > - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> > + uint16_t tmp;
> > + int ret = parse_uint16_t(&tmp, arg);
> >
> > if (ret) {
> > RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> > return -1;
> > }
> >
> > + test_data->window_sz = (int)tmp;
> > return 0;
> > }
> [Fiona] I expect this fixes this coverity issue - but will it result in another one?
> window_sz on the xform is uint8_t - so this int will get truncated later, and
> there's no cast done at that point.
> Would it be better to add a new parse_uint8_t fn and change test-data-
> >window_sz to a unit8_t?
> Or add that cast?
> [Tomek] I measn it's ok. There's a check inside comp_perf_check_capabilities function. If the value from test_data->window_sz > cap->window_size we have a fail. Also during parsing there's a check is value from command line is between 0 and UINT16_MAX, so in my opinion all cases are tested. The point is there's only one place where we're parsing uint8_t value. parse_uint8_t function will be especially for that.
Hi Fiona,
Outlook issue :D , so once again
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, May 20, 2019 4:06 PM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; dev@dpdk.org;
> shallyv@marvell.com; stable@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Trybula, ArturX
> <arturx.trybula@intel.com>
> Subject: RE: [PATCH] app/test-compress-perf: fix reliance on integer
> endianness
>
> HI Tomasz,
>
> > -----Original Message-----
> > From: Jozwiak, TomaszX
> > Sent: Monday, May 20, 2019 2:26 PM
> > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak,
> > TomaszX <tomaszx.jozwiak@intel.com>; shallyv@marvell.com;
> > stable@dpdk.org
> > Subject: [PATCH] app/test-compress-perf: fix reliance on integer
> > endianness
> >
> > This patch fixes coverity issue:
> > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> parse_window_sz
> > function.
> >
> > Coverity issue: 328524
> > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > ---
> > app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > b/app/test-compress- perf/comp_perf_options_parse.c index
> > 2fb6fb4..56ca580 100644
> > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> comp_test_data
> > *test_data, const char *arg) static int parse_window_sz(struct
> > comp_test_data *test_data, const char *arg) {
> > - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> > + uint16_t tmp;
> > + int ret = parse_uint16_t(&tmp, arg);
> >
> > if (ret) {
> > RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> > return -1;
> > }
> >
> > + test_data->window_sz = (int)tmp;
> > return 0;
> > }
> [Fiona] I expect this fixes this coverity issue - but will it result in another one?
> window_sz on the xform is uint8_t - so this int will get truncated later, and
> there's no cast done at that point.
> Would it be better to add a new parse_uint8_t fn and change test-data-
> >window_sz to a unit8_t?
> Or add that cast?
[Tomek] I measn it's ok. There's a check inside comp_perf_check_capabilities function.
If the value from test_data->window_sz > cap->window_size we have a fail.
Also during parsing there's a check is value from command line between 0 and UINT16_MAX,
so in my opinion all cases are tested. The point is there's only one place where
we're parsing uint8_t value. parse_uint8_t function will be especially for that.
HI Tomasz
> -----Original Message-----
> From: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Sent: Tuesday, May 21, 2019 12:18 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; Shally Verma
> <shallyv@marvell.com>; stable@dpdk.org
> Cc: Trybula, ArturX <arturx.trybula@intel.com>
> Subject: [EXT] RE: [PATCH] app/test-compress-perf: fix reliance on integer
> endianness
>
> External Email
>
> ----------------------------------------------------------------------
> Hi Fiona,
>
> Outlook issue :D , so once again
>
> > -----Original Message-----
> > From: Trahe, Fiona
> > Sent: Monday, May 20, 2019 4:06 PM
> > To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; dev@dpdk.org;
> > shallyv@marvell.com; stable@dpdk.org
> > Cc: Trahe, Fiona <fiona.trahe@intel.com>; Trybula, ArturX
> > <arturx.trybula@intel.com>
> > Subject: RE: [PATCH] app/test-compress-perf: fix reliance on integer
> > endianness
> >
> > HI Tomasz,
> >
> > > -----Original Message-----
> > > From: Jozwiak, TomaszX
> > > Sent: Monday, May 20, 2019 2:26 PM
> > > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak,
> > > TomaszX <tomaszx.jozwiak@intel.com>; shallyv@marvell.com;
> > > stable@dpdk.org
> > > Subject: [PATCH] app/test-compress-perf: fix reliance on integer
> > > endianness
> > >
> > > This patch fixes coverity issue:
> > > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> > parse_window_sz
> > > function.
> > >
> > > Coverity issue: 328524
> > > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > > ---
> > > app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > > b/app/test-compress- perf/comp_perf_options_parse.c index
> > > 2fb6fb4..56ca580 100644
> > > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> > comp_test_data
> > > *test_data, const char *arg) static int parse_window_sz(struct
> > > comp_test_data *test_data, const char *arg) {
> > > - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> > > + uint16_t tmp;
> > > + int ret = parse_uint16_t(&tmp, arg);
> > >
> > > if (ret) {
> > > RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> > > return -1;
> > > }
> > >
> > > + test_data->window_sz = (int)tmp;
> > > return 0;
> > > }
> > [Fiona] I expect this fixes this coverity issue - but will it result in another one?
> > window_sz on the xform is uint8_t - so this int will get truncated
> > later, and there's no cast done at that point.
> > Would it be better to add a new parse_uint8_t fn and change test-data-
> > >window_sz to a unit8_t?
> > Or add that cast?
> [Tomek] I measn it's ok. There's a check inside comp_perf_check_capabilities
> function.
> If the value from test_data->window_sz > cap->window_size we have a fail.
> Also during parsing there's a check is value from command line between 0 and
> UINT16_MAX, so in my opinion all cases are tested. The point is there's only
> one place where we're parsing uint8_t value. parse_uint8_t function will be
> especially for that.
[Shally] What is window_sz in test data ?is it base 2 log of (actual window length) or actual window length in bytes? lib spec mention this as struct rte_param_log2_range, so
If test window size is actual window length in bytes then I assume test perf should check for test_data->window_sz > 2 pow cap->window_size but that doesn't look like the case.
So if it is log value, then coding wise typecasting here doesn't look right. Though it add need for extra function to parse_uint8, but that looks like cleaner approach to use.
Hi Shally
> -----Original Message-----
> From: Shally Verma [mailto:shallyv@marvell.com]
> Sent: Tuesday, May 21, 2019 10:04 AM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; dev@dpdk.org; stable@dpdk.org
> Cc: Trybula, ArturX <arturx.trybula@intel.com>
> Subject: RE: [PATCH] app/test-compress-perf: fix reliance on integer
> endianness
>
> HI Tomasz
>
> > -----Original Message-----
> > From: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> > Sent: Tuesday, May 21, 2019 12:18 PM
> > To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; Shally Verma
> > <shallyv@marvell.com>; stable@dpdk.org
> > Cc: Trybula, ArturX <arturx.trybula@intel.com>
> > Subject: [EXT] RE: [PATCH] app/test-compress-perf: fix reliance on
> > integer endianness
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > Hi Fiona,
> >
> > Outlook issue :D , so once again
> >
> > > -----Original Message-----
> > > From: Trahe, Fiona
> > > Sent: Monday, May 20, 2019 4:06 PM
> > > To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; dev@dpdk.org;
> > > shallyv@marvell.com; stable@dpdk.org
> > > Cc: Trahe, Fiona <fiona.trahe@intel.com>; Trybula, ArturX
> > > <arturx.trybula@intel.com>
> > > Subject: RE: [PATCH] app/test-compress-perf: fix reliance on integer
> > > endianness
> > >
> > > HI Tomasz,
> > >
> > > > -----Original Message-----
> > > > From: Jozwiak, TomaszX
> > > > Sent: Monday, May 20, 2019 2:26 PM
> > > > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak,
> > > > TomaszX <tomaszx.jozwiak@intel.com>; shallyv@marvell.com;
> > > > stable@dpdk.org
> > > > Subject: [PATCH] app/test-compress-perf: fix reliance on integer
> > > > endianness
> > > >
> > > > This patch fixes coverity issue:
> > > > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> > > parse_window_sz
> > > > function.
> > > >
> > > > Coverity issue: 328524
> > > > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > > > ---
> > > > app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > > > b/app/test-compress- perf/comp_perf_options_parse.c index
> > > > 2fb6fb4..56ca580 100644
> > > > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > > > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > > > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> > > comp_test_data
> > > > *test_data, const char *arg) static int parse_window_sz(struct
> > > > comp_test_data *test_data, const char *arg) {
> > > > - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> > > > + uint16_t tmp;
> > > > + int ret = parse_uint16_t(&tmp, arg);
> > > >
> > > > if (ret) {
> > > > RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> > > > return -1;
> > > > }
> > > >
> > > > + test_data->window_sz = (int)tmp;
> > > > return 0;
> > > > }
> > > [Fiona] I expect this fixes this coverity issue - but will it result in another
> one?
> > > window_sz on the xform is uint8_t - so this int will get truncated
> > > later, and there's no cast done at that point.
> > > Would it be better to add a new parse_uint8_t fn and change
> > > test-data-
> > > >window_sz to a unit8_t?
> > > Or add that cast?
> > [Tomek] I measn it's ok. There's a check inside
> > comp_perf_check_capabilities function.
> > If the value from test_data->window_sz > cap->window_size we have a
> fail.
> > Also during parsing there's a check is value from command line between
> > 0 and UINT16_MAX, so in my opinion all cases are tested. The point is
> > there's only one place where we're parsing uint8_t value.
> > parse_uint8_t function will be especially for that.
> [Shally] What is window_sz in test data ?is it base 2 log of (actual window
> length) or actual window length in bytes? lib spec mention this as struct
> rte_param_log2_range, so If test window size is actual window length in
> bytes then I assume test perf should check for test_data->window_sz > 2
> pow cap->window_size but that doesn't look like the case.
> So if it is log value, then coding wise typecasting here doesn't look right.
> Though it add need for extra function to parse_uint8, but that looks like
> cleaner approach to use.
[Tomek] I mean it's log 2 (please take a look at help usage function in comp_perf_options_parse.c:37
" --window-sz N: base two log value of compression window size\n"
" (e.g.: 15 => 32k, default: max supported by PMD)\n"
I mean it's ok, still. We have many types in command line and can be much more in the future. The idea is to parse them into a sort of common range value first ( it should be max range for all digital command line options - in our case there's uint16 or uint32_t) even if it's shorter like uint8_t or etc. We store these values in comp_test_data structure first. Next we check the ranges each of them. In case of window_sz this test is in comp_perf_check_capabilities function. That approach reduce a set of parsing functions we needed. Of course we can create separate parsing function for each of command line type value, but is this really needed ? :D
Please let me know your thoughts - if this new parsing function will clear the code - I'll add this in v2
Hi Shally, Tomasz,
> > > > > -----Original Message-----
> > > > > From: Jozwiak, TomaszX
> > > > > Sent: Monday, May 20, 2019 2:26 PM
> > > > > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak,
> > > > > TomaszX <tomaszx.jozwiak@intel.com>; shallyv@marvell.com;
> > > > > stable@dpdk.org
> > > > > Subject: [PATCH] app/test-compress-perf: fix reliance on integer
> > > > > endianness
> > > > >
> > > > > This patch fixes coverity issue:
> > > > > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> > > > parse_window_sz
> > > > > function.
> > > > >
> > > > > Coverity issue: 328524
> > > > > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > > > > ---
> > > > > app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > > > > b/app/test-compress- perf/comp_perf_options_parse.c index
> > > > > 2fb6fb4..56ca580 100644
> > > > > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > > > > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > > > > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> > > > comp_test_data
> > > > > *test_data, const char *arg) static int parse_window_sz(struct
> > > > > comp_test_data *test_data, const char *arg) {
> > > > > - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> > > > > + uint16_t tmp;
> > > > > + int ret = parse_uint16_t(&tmp, arg);
> > > > >
> > > > > if (ret) {
> > > > > RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> > > > > return -1;
> > > > > }
> > > > >
> > > > > + test_data->window_sz = (int)tmp;
> > > > > return 0;
> > > > > }
> > > > [Fiona] I expect this fixes this coverity issue - but will it result in another
> > one?
> > > > window_sz on the xform is uint8_t - so this int will get truncated
> > > > later, and there's no cast done at that point.
> > > > Would it be better to add a new parse_uint8_t fn and change
> > > > test-data-
> > > > >window_sz to a unit8_t?
> > > > Or add that cast?
> > > [Tomek] I measn it's ok. There's a check inside
> > > comp_perf_check_capabilities function.
> > > If the value from test_data->window_sz > cap->window_size we have a
> > fail.
> > > Also during parsing there's a check is value from command line between
> > > 0 and UINT16_MAX, so in my opinion all cases are tested. The point is
> > > there's only one place where we're parsing uint8_t value.
> > > parse_uint8_t function will be especially for that.
> > [Shally] What is window_sz in test data ?is it base 2 log of (actual window
> > length) or actual window length in bytes? lib spec mention this as struct
> > rte_param_log2_range, so If test window size is actual window length in
> > bytes then I assume test perf should check for test_data->window_sz > 2
> > pow cap->window_size but that doesn't look like the case.
> > So if it is log value, then coding wise typecasting here doesn't look right.
> > Though it add need for extra function to parse_uint8, but that looks like
> > cleaner approach to use.
> [Tomek] I mean it's log 2 (please take a look at help usage function in comp_perf_options_parse.c:37
>
> " --window-sz N: base two log value of compression window size\n"
> " (e.g.: 15 => 32k, default: max supported by PMD)\n"
>
> I mean it's ok, still. We have many types in command line and can be much more in the future. The idea
> is to parse them into a sort of common range value first ( it should be max range for all digital command
> line options - in our case there's uint16 or uint32_t) even if it's shorter like uint8_t or etc. We store
> these values in comp_test_data structure first. Next we check the ranges each of them. In case of
> window_sz this test is in comp_perf_check_capabilities function. That approach reduce a set of parsing
> functions we needed. Of course we can create separate parsing function for each of command line type
> value, but is this really needed ? :D
> Please let me know your thoughts - if this new parsing function will clear the code - I'll add this in v2
[Fiona] ok, I reviewed again and see I'd misunderstood.
The param being parsed is intentionally not being stored in test_data struct as uint8_t, but as
an int because it uses -1 as a default value. And there are range checks on the input, so an invalid value will never be passed to the PMD.
So I'm ok with the fix as is - it resolves the coverity issues reported on the param parsing.
However there's a second issue, which coverity is likely to throw up after the above fix is applied - when
the test_data value is later passed to the PMD, it should have a cast from int to unit8_t.
But that's a separate issue, not referred to by this coverity report, so we'll send a separate patch for it.
@Shally, are you ok with this?
> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: Wednesday, May 29, 2019 7:17 PM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; Shally Verma
> <shallyv@marvell.com>; dev@dpdk.org; stable@dpdk.org
> Cc: Trybula, ArturX <arturx.trybula@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>
> Subject: [EXT] RE: [PATCH] app/test-compress-perf: fix reliance on integer
> endianness
>
> External Email
>
> ----------------------------------------------------------------------
> Hi Shally, Tomasz,
>
> > > > > > -----Original Message-----
> > > > > > From: Jozwiak, TomaszX
> > > > > > Sent: Monday, May 20, 2019 2:26 PM
> > > > > > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>;
> > > > > > Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>;
> > > > > > shallyv@marvell.com; stable@dpdk.org
> > > > > > Subject: [PATCH] app/test-compress-perf: fix reliance on
> > > > > > integer endianness
> > > > > >
> > > > > > This patch fixes coverity issue:
> > > > > > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> > > > > parse_window_sz
> > > > > > function.
> > > > > >
> > > > > > Coverity issue: 328524
> > > > > > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > > > > > ---
> > > > > > app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > > > > > b/app/test-compress- perf/comp_perf_options_parse.c index
> > > > > > 2fb6fb4..56ca580 100644
> > > > > > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > > > > > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > > > > > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> > > > > comp_test_data
> > > > > > *test_data, const char *arg) static int
> > > > > > parse_window_sz(struct comp_test_data *test_data, const char *arg)
> {
> > > > > > - int ret = parse_uint16_t((uint16_t *)&test_data->window_sz,
> arg);
> > > > > > + uint16_t tmp;
> > > > > > + int ret = parse_uint16_t(&tmp, arg);
> > > > > >
> > > > > > if (ret) {
> > > > > > RTE_LOG(ERR, USER1, "Failed to parse window
> size\n");
> > > > > > return -1;
> > > > > > }
> > > > > >
> > > > > > + test_data->window_sz = (int)tmp;
> > > > > > return 0;
> > > > > > }
> > > > > [Fiona] I expect this fixes this coverity issue - but will it
> > > > > result in another
> > > one?
> > > > > window_sz on the xform is uint8_t - so this int will get
> > > > > truncated later, and there's no cast done at that point.
> > > > > Would it be better to add a new parse_uint8_t fn and change
> > > > > test-data-
> > > > > >window_sz to a unit8_t?
> > > > > Or add that cast?
> > > > [Tomek] I measn it's ok. There's a check inside
> > > > comp_perf_check_capabilities function.
> > > > If the value from test_data->window_sz > cap->window_size we have
> > > > a
> > > fail.
> > > > Also during parsing there's a check is value from command line
> > > > between
> > > > 0 and UINT16_MAX, so in my opinion all cases are tested. The point
> > > > is there's only one place where we're parsing uint8_t value.
> > > > parse_uint8_t function will be especially for that.
> > > [Shally] What is window_sz in test data ?is it base 2 log of (actual
> > > window
> > > length) or actual window length in bytes? lib spec mention this as
> > > struct rte_param_log2_range, so If test window size is actual window
> > > length in bytes then I assume test perf should check for
> > > test_data->window_sz > 2 pow cap->window_size but that doesn't look like
> the case.
> > > So if it is log value, then coding wise typecasting here doesn't look right.
> > > Though it add need for extra function to parse_uint8, but that looks
> > > like cleaner approach to use.
> > [Tomek] I mean it's log 2 (please take a look at help usage function
> > in comp_perf_options_parse.c:37
> >
> > " --window-sz N: base two log value of compression window size\n"
> > " (e.g.: 15 => 32k, default: max supported by
> PMD)\n"
> >
> > I mean it's ok, still. We have many types in command line and can be
> > much more in the future. The idea is to parse them into a sort of
> > common range value first ( it should be max range for all digital
> > command line options - in our case there's uint16 or uint32_t) even if
> > it's shorter like uint8_t or etc. We store these values in
> > comp_test_data structure first. Next we check the ranges each of them.
> > In case of window_sz this test is in comp_perf_check_capabilities
> > function. That approach reduce a set of parsing functions we needed.
> > Of course we can create separate parsing function for each of command
> > line type value, but is this really needed ? :D Please let me know
> > your thoughts - if this new parsing function will clear the code -
> > I'll add this in v2
> [Fiona] ok, I reviewed again and see I'd misunderstood.
> The param being parsed is intentionally not being stored in test_data struct as
> uint8_t, but as an int because it uses -1 as a default value. And there are range
> checks on the input, so an invalid value will never be passed to the PMD.
> So I'm ok with the fix as is - it resolves the coverity issues reported on the param
> parsing.
>
> However there's a second issue, which coverity is likely to throw up after the
> above fix is applied - when the test_data value is later passed to the PMD, it
> should have a cast from int to unit8_t.
> But that's a separate issue, not referred to by this coverity report, so we'll send a
> separate patch for it.
> @Shally, are you ok with this?
[Shally] No hard choices here. But why we cant app use 0 as default value instead of -1? It would save us an additional typecast when passing to PMD
Thanks
Shally
@@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct comp_test_data *test_data, const char *arg)
static int
parse_window_sz(struct comp_test_data *test_data, const char *arg)
{
- int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
+ uint16_t tmp;
+ int ret = parse_uint16_t(&tmp, arg);
if (ret) {
RTE_LOG(ERR, USER1, "Failed to parse window size\n");
return -1;
}
+ test_data->window_sz = (int)tmp;
return 0;
}