| Message ID | 20180830205850.10352-2-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | More oss-fuzz fixes | expand |
Thanks for the fix, looks good to me. Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> On Thu, Aug 30, 2018 at 1:59 PM Ben Pfaff <blp@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 <blp@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks. I applied these patches to master. On Mon, Sep 10, 2018 at 11:26:54AM -0700, Yifeng Sun wrote: > Thanks for the fix, looks good to me. > > Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> > > On Thu, Aug 30, 2018 at 1:59 PM Ben Pfaff <blp@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 <blp@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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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); } }
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: Ben Pfaff <blp@ovn.org> --- lib/ofp-port.c | 9 ++++++++- lib/ofp-print.c | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)