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 <ho...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
