On Thu, Sep 05, 2013 at 12:58:40PM -0700, Ben Pfaff wrote:
> 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, that seems like a good approach to me.
Perhaps we should remove 'gs.bucket_stats = NULL;' from
ofp_print_group_stats() as it seems to be unnecessary now.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
