[ovs-dev] [PATCH v3 2/5] ovs-ofctl: Handle any number of buckets in group statistics

[Ben Pfaff]

On Thu, Sep 05, 2013 at 12:55:28PM -0700, Ben Pfaff wrote:
> On Thu, Sep 05, 2013 at 02:19:13PM +0900, Simon Horman wrote:
> > struct ofputil_group_stats has an arbitrary limit
> > of 16 buckets for which it can record statistics.
> > However the code does not appear to enforce this
> > limit and it seems to me that the code could overflow.
> > 
> > This patch aims to remove the arbitrary limit by
> > changing the 'bucket_stats' field of struct ofputil_group_stats
> > from a fixed length array to a pointer whose storage is allocated and freed
> > as necessary.
> > 
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> > 
> > ---
> > v3
> > * As suggested by Ben Pfaff
> >   - Vastly simplify the change by using an explicit pointer for
> >     the 'bucket_stats' member of struct ofputil_group_stats rather
> >     than implicit variable length array appended to the end of the
> >     structure.
> 
> Thanks!
> 
> It looked to me that ofp_print_group_stats() shouldn't free
> gs.bucket_stats on error because in that case gs.bucket_stats won't be
> properly initialized, so I changed that.  And then I changed
> ofputil_decode_group_stats_reply() to always initialize
> gs.bucket_stats as a second measure of protection.
> 
> I folded in the following and will apply patches 1 and 2 soon.
> 
> I'll review patches 3 through 5 tomorrow during the hackathon.
> (Thanks so much for participating!)
> 
> --8<--------------------------cut here-------------------------->8--
> 
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index a794e70..6433dd0 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -2229,7 +2229,6 @@ ofp_print_group_stats(struct ds *s, const struct ofp_header *oh)
>              if (retval != EOF) {
>                  ds_put_cstr(s, " ***parse error***");
>              }
> -            free(gs.bucket_stats);
>              break;
>          }
>  
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 6573025..8ac9186 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -5480,7 +5480,8 @@ ofputil_decode_group_stats_request(const struct ofp_header *request,
>  }
>  
>  /* Converts a group stats reply in 'msg' into an abstract ofputil_group_stats
> - * in 'gs'.
> + * in 'gs'.  Assigns freshly allocated memory to gs->bucket_stats for the
> + * caller to eventually free.
>   *
>   * Multiple group stats replies can be packed into a single OpenFlow message.
>   * Calling this function multiple times for a single 'msg' iterates through the
> @@ -5501,6 +5502,7 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
>      size_t length;
>      size_t i;
>  
> +    gs->bucket_stats = NULL;
>      error = (msg->l2
>               ? ofpraw_decode(&raw, msg->l2)
>               : ofpraw_pull(&raw, msg));

Oh, I see now that you had 'gs.bucket_stats = NULL;' in
ofp_print_group_stats() just before calling
ofputil_decode_group_stats_reply(), so this wasn't actually a bug.
Still, I think it's better to put it in
ofputil_decode_group_stats_reply() itself.

Thanks,

Ben.