On Thu, Sep 05, 2013 at 08:52:40PM -0700, Ben Pfaff wrote:
> On Fri, Sep 06, 2013 at 10:57:26AM +0900, Simon Horman wrote:
> > 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.
>
> I did that before I pushed the commit, but I forgot to mention it here.
Excellent, thanks.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
