[dpdk-dev,3/5] example_ip_pipeline: fix sizeof() on memcpy

  Found by Coverity:
  Sizeof not portable (SIZEOF_MISMATCH)
  suspicious_sizeof: Passing argument &app->cmds[app->n_cmds] of type
  cmdline_parse_ctx_t * and argument n_cmds * 8UL /* sizeof (cmdline_parse_ctx_t *) */
  to function memcpy is suspicious.
   In this case, sizeof (cmdline_parse_ctx_t *) is equal to sizeof (cmdline_parse_ctx_t),
   but this is not a portable assumption.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 examples/ip_pipeline/init.c                                  | 2 +-
 examples/ip_pipeline/pipeline/pipeline_common_fe.c           | 2 +-
 examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)
  

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, September 1, 2015 4:59 AM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: [PATCH 3/5] example_ip_pipeline: fix sizeof() on memcpy
> 
> Found by Coverity:
>   Sizeof not portable (SIZEOF_MISMATCH)
>   suspicious_sizeof: Passing argument &app->cmds[app->n_cmds] of type
>   cmdline_parse_ctx_t * and argument n_cmds * 8UL /* sizeof
> (cmdline_parse_ctx_t *) */
>   to function memcpy is suspicious.
>    In this case, sizeof (cmdline_parse_ctx_t *) is equal to sizeof
> (cmdline_parse_ctx_t),
>    but this is not a portable assumption.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  examples/ip_pipeline/init.c                                  | 2 +-
>  examples/ip_pipeline/pipeline/pipeline_common_fe.c           | 2 +-
>  examples/ip_pipeline/pipeline/pipeline_flow_classification.c | 1 -
>  3 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
> index 3f9c68d..75e3767 100644
> --- a/examples/ip_pipeline/init.c
> +++ b/examples/ip_pipeline/init.c
> @@ -1325,7 +1325,7 @@ app_pipeline_type_cmd_push(struct app_params
> *app,
>  	/* Push pipeline commands into the application */
>  	memcpy(&app->cmds[app->n_cmds],
>  		cmds,
> -		n_cmds * sizeof(cmdline_parse_ctx_t *));
> +		n_cmds * sizeof(cmdline_parse_ctx_t));

Actually no, as we are both the destination and the source of memcpy are array of pointers.

The source is a pipeline type, which is a static global data structure, so no issues with the life time of the data pointed to by the pointers.

> 
>  	for (i = 0; i < n_cmds; i++)
>  		app->cmds[app->n_cmds + i]->data = app;
> diff --git a/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> b/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> index fcda0ce..4eec66b 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_common_fe.c
> @@ -1321,7 +1321,7 @@ app_pipeline_common_cmd_push(struct
> app_params *app)
>  	/* Push pipeline commands into the application */
>  	memcpy(&app->cmds[app->n_cmds],
>  		pipeline_common_cmds,
> -		n_cmds * sizeof(cmdline_parse_ctx_t *));
> +		n_cmds * sizeof(cmdline_parse_ctx_t));

Actually no, as we are both the destination and the source of memcpy are array of pointers.

The source is a pipeline type, which is a static global data structure, so no issues with the life time of the data pointed to by the pointers.

> 
>  	for (i = 0; i < n_cmds; i++)
>  		app->cmds[app->n_cmds + i]->data = app;
> diff --git a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> index 24cf7dc..e5141b0 100644
> --- a/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> +++ b/examples/ip_pipeline/pipeline/pipeline_flow_classification.c
> @@ -126,7 +126,6 @@ app_pipeline_fc_key_convert(struct pipeline_fc_key
> *key_in,
>  	{
>  		struct pkt_key_ipv6_5tuple *ipv6 = key_buffer;
> 
> -		memset(ipv6, 0, 64);

Agreed!

>  		ipv6->payload_length = 0;
>  		ipv6->proto = key_in->key.ipv6_5tuple.proto;
>  		ipv6->hop_limit = 0;
> --
> 2.1.4
  

On Wed, 9 Sep 2015 18:25:53 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> > diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
> > index 3f9c68d..75e3767 100644
> > --- a/examples/ip_pipeline/init.c
> > +++ b/examples/ip_pipeline/init.c
> > @@ -1325,7 +1325,7 @@ app_pipeline_type_cmd_push(struct app_params
> > *app,
> >  	/* Push pipeline commands into the application */
> >  	memcpy(&app->cmds[app->n_cmds],
> >  		cmds,
> > -		n_cmds * sizeof(cmdline_parse_ctx_t *));
> > +		n_cmds * sizeof(cmdline_parse_ctx_t));  
> 
> Actually no, as we are both the destination and the source of memcpy are array of pointers.
> 
> The source is a pipeline type, which is a static global data structure, so no issues with the life time of the data pointed to by the pointers.

In order to make tools happy, shouldn't the source and target be pointers to array of pointers.
In the current code
  &app->cmd[app->n_cmds] is type cmdline_parse_ctx_t *
  cmds is type cmdline_parse_ctx_t *

And type cmdline_parse_ctx_t is already a pointer.

Copying a set of pointers to pointers vs set of pointers will be the same since both are
the same size, but static checking tools see the problem.

This is why kernel developers particularly despise typedefs which hide
pointers like this one.