[ovs-dev] [PATCH v2 1/2] ofp-port: Further cleanups and fixes for ofputil_decode_port_stats().

[Yifeng Sun]

Thanks for the fix, looks good to me.

Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>

On Thu, Aug 30, 2018 at 1:59 PM Ben Pfaff <blp at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>