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

[Simon Horman]

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.

v2
* No change
---
 lib/ofp-print.c   | 4 ++++
 lib/ofp-util.c    | 1 +
 lib/ofp-util.h    | 2 +-
 ofproto/ofproto.c | 4 ++++
 4 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 702d9c1..a794e70 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2223,11 +2223,13 @@ ofp_print_group_stats(struct ds *s, const struct ofp_header *oh)
         struct ofputil_group_stats gs;
         int retval;
 
+        gs.bucket_stats = NULL;
         retval = ofputil_decode_group_stats_reply(&b, &gs);
         if (retval) {
             if (retval != EOF) {
                 ds_put_cstr(s, " ***parse error***");
             }
+            free(gs.bucket_stats);
             break;
         }
 
@@ -2252,6 +2254,8 @@ ofp_print_group_stats(struct ds *s, const struct ofp_header *oh)
                 ds_put_format(s, "byte_count=%"PRIu64"", gs.bucket_stats[bucket_i].byte_count);
             }
         }
+
+        free(gs.bucket_stats);
      }
 }
 
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index b181a0d..6573025 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5557,6 +5557,7 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
         return OFPERR_OFPBRC_BAD_LEN;
     }
 
+    gs->bucket_stats = xmalloc(gs->n_buckets * sizeof *gs->bucket_stats);
     for (i = 0; i < gs->n_buckets; i++) {
         gs->bucket_stats[i].packet_count = ntohll(obc[i].packet_count);
         gs->bucket_stats[i].byte_count = ntohll(obc[i].byte_count);
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 6de2a05..eae8ed5 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -913,7 +913,7 @@ struct ofputil_group_stats {
     uint32_t duration_sec;      /* UINT32_MAX if unknown. */
     uint32_t duration_nsec;
     uint32_t n_buckets;
-    struct bucket_counter bucket_stats[16];
+    struct bucket_counter *bucket_stats;
 };
 
 /* Group features reply, independent of protocol. */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dacbb71..b94fd8e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4786,6 +4786,8 @@ append_group_stats(struct ofgroup *group, struct list *replies)
     long long int now = time_msec();
     int error;
 
+    ogs.bucket_stats = xmalloc(group->n_buckets * sizeof *ogs.bucket_stats);
+
     error = (ofproto->ofproto_class->group_get_stats
              ? ofproto->ofproto_class->group_get_stats(group, &ogs)
              : EOPNOTSUPP);
@@ -4802,6 +4804,8 @@ append_group_stats(struct ofgroup *group, struct list *replies)
     calc_duration(group->created, now, &ogs.duration_sec, &ogs.duration_nsec);
 
     ofputil_append_group_stats(replies, &ogs);
+
+    free(ogs.bucket_stats);
 }
 
 static enum ofperr
-- 
1.8.4