Thanks for the fix, looks good to me. Reviewed-by: Yifeng Sun <pkusunyif...@gmail.com>
On Thu, Aug 30, 2018 at 1:59 PM Ben Pfaff <b...@ovn.org> wrote: > This fixes leaks on the error path in parse_intel_port_custom_property(). > > ofp_print_ofpst_port_reply() failed to free the custom_stats in decoded > port stats. This fixes the problem. > > parse_intel_port_custom_property() had a memory leak if there was more than > one custom stats property (which there shouldn't be, but still). This > fixes the problem. > > There was a function netdev_free_custom_stats_counters() meant for freeing > custom_stats, but hardly anything used it. This adopts it consistently. > > It wasn't safe to free the custom stats if ofputil_decode_port_stats() > returned an error. Using netdev_free_custom_stats_counters() avoids this > pitfall. > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9972 > Signed-off-by > <https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9972Signed-off-by>: > Ben Pfaff <b...@ovn.org> > --- > lib/ofp-port.c | 9 ++++++++- > lib/ofp-print.c | 1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/lib/ofp-port.c b/lib/ofp-port.c > index 8d882a14b4df..ec70f46e96bb 100644 > --- a/lib/ofp-port.c > +++ b/lib/ofp-port.c > @@ -1608,6 +1608,7 @@ parse_intel_port_custom_property(struct ofpbuf > *payload, > > ops->custom_stats.size = ntohs(custom_stats->stats_array_size); > > + netdev_free_custom_stats_counters(&ops->custom_stats); > ops->custom_stats.counters = xcalloc(ops->custom_stats.size, > sizeof > *ops->custom_stats.counters); > > @@ -1618,6 +1619,7 @@ parse_intel_port_custom_property(struct ofpbuf > *payload, > uint8_t *name_len = ofpbuf_try_pull(payload, sizeof *name_len); > char *name = name_len ? ofpbuf_try_pull(payload, *name_len) : > NULL; > if (!name_len || !name) { > + netdev_free_custom_stats_counters(&ops->custom_stats); > return OFPERR_OFPBPC_BAD_LEN; > } > > @@ -1628,6 +1630,7 @@ parse_intel_port_custom_property(struct ofpbuf > *payload, > /* Counter value. */ > ovs_be64 *value = ofpbuf_try_pull(payload, sizeof *value); > if (!value) { > + netdev_free_custom_stats_counters(&ops->custom_stats); > return OFPERR_OFPBPC_BAD_LEN; > } > c->value = ntohll(get_unaligned_be64(value)); > @@ -1714,6 +1717,7 @@ ofputil_count_port_stats(const struct ofp_header *oh) > if (ofputil_decode_port_stats(&ps, &b)) { > return n; > } > + netdev_free_custom_stats_counters(&ps.custom_stats); > } > } > > @@ -1726,7 +1730,10 @@ ofputil_count_port_stats(const struct ofp_header > *oh) > * null and not modify them between calls. > * > * Returns 0 if successful, EOF if no replies were left in this 'msg', > - * otherwise a positive errno value. */ > + * otherwise a positive errno value. > + * > + * On success, the caller must eventually free ps->custom_stats.counters, > + * with netdev_free_custom_stats_counters(&ps->custom_stats). */ > int > ofputil_decode_port_stats(struct ofputil_port_stats *ps, struct ofpbuf > *msg) > { > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index cf93d2e2cb38..e05a969a82b0 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -561,6 +561,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const > struct ofp_header *oh, > return retval != EOF ? retval : 0; > } > ofputil_format_port_stats(string, &ps, port_map); > + netdev_free_custom_stats_counters(&ps.custom_stats); > } > } > > -- > 2.16.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
