| Message ID | 1536844795-2926-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofp-print: Fix a memory leak reported by fuzz | expand |
Thanks for the fix, Yifeng. I pushed this to master and branch-2.10. However, after I pushed it, I noticed a couple of issues:
- There is at least one more code path that would have a similar issue.
- The description of ofputil_decode_port_stats() states that it only needs to be freed on success.
We could fix both of those issues, but I think the leak happens when an error happens after a successful call to parse_intel_port_custom_property() in ofputil_pull_ofp14_port_stats(). What about something like the incremental patch at the end of this message instead?
--Justin
-=-=-=-=-=-=-=-=-
diff --git a/lib/ofp-port.c b/lib/ofp-port.c
index ec70f46e96bb..3d1ada9ceb99 100644
--- a/lib/ofp-port.c
+++ b/lib/ofp-port.c
@@ -1697,6 +1697,7 @@ ofputil_pull_ofp14_port_stats(struct ofputil_port_stats *ops,
}
if (error) {
+ netdev_free_custom_stats_counters(&ops->custom_stats);
return error;
}
}
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 37d7b8b98c55..e05a969a82b0 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -558,7 +558,6 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh,
retval = ofputil_decode_port_stats(&ps, &b);
if (retval) {
- netdev_free_custom_stats_counters(&ps.custom_stats);
return retval != EOF ? retval : 0;
}
ofputil_format_port_stats(string, &ps, port_map);
> On Sep 13, 2018, at 6:19 AM, Yifeng Sun <pkusunyifeng@gmail.com> wrote:
>
> When ofputil_decode_port_stats returns error, it is possible that
> custom_stats_counters is valid and still need freed.
>
> The fuzz report link is
> https://oss-fuzz.com/testcase?key=5739356233400320
>
> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> ---
> lib/ofp-print.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index e05a969a82b0..37d7b8b98c55 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -558,6 +558,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh,
>
> retval = ofputil_decode_port_stats(&ps, &b);
> if (retval) {
> + netdev_free_custom_stats_counters(&ps.custom_stats);
> return retval != EOF ? retval : 0;
> }
> ofputil_format_port_stats(string, &ps, port_map);
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks Justin. Your attached fix is better. Yifeng On Fri, Sep 14, 2018 at 1:14 PM Justin Pettit <jpettit@ovn.org> wrote: > Thanks for the fix, Yifeng. I pushed this to master and branch-2.10. > However, after I pushed it, I noticed a couple of issues: > > - There is at least one more code path that would have a similar > issue. > > - The description of ofputil_decode_port_stats() states that it > only needs to be freed on success. > > We could fix both of those issues, but I think the leak happens when an > error happens after a successful call to parse_intel_port_custom_property() > in ofputil_pull_ofp14_port_stats(). What about something like the > incremental patch at the end of this message instead? > > --Justin > > > -=-=-=-=-=-=-=-=- > > diff --git a/lib/ofp-port.c b/lib/ofp-port.c > index ec70f46e96bb..3d1ada9ceb99 100644 > --- a/lib/ofp-port.c > +++ b/lib/ofp-port.c > @@ -1697,6 +1697,7 @@ ofputil_pull_ofp14_port_stats(struct > ofputil_port_stats *ops, > } > > if (error) { > + netdev_free_custom_stats_counters(&ops->custom_stats); > return error; > } > } > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index 37d7b8b98c55..e05a969a82b0 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -558,7 +558,6 @@ ofp_print_ofpst_port_reply(struct ds *string, const > struct ofp_header *oh, > > retval = ofputil_decode_port_stats(&ps, &b); > if (retval) { > - netdev_free_custom_stats_counters(&ps.custom_stats); > return retval != EOF ? retval : 0; > } > ofputil_format_port_stats(string, &ps, port_map); > > > > > On Sep 13, 2018, at 6:19 AM, Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > > > When ofputil_decode_port_stats returns error, it is possible that > > custom_stats_counters is valid and still need freed. > > > > The fuzz report link is > > https://oss-fuzz.com/testcase?key=5739356233400320 > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > --- > > lib/ofp-print.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > > index e05a969a82b0..37d7b8b98c55 100644 > > --- a/lib/ofp-print.c > > +++ b/lib/ofp-print.c > > @@ -558,6 +558,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const > struct ofp_header *oh, > > > > retval = ofputil_decode_port_stats(&ps, &b); > > if (retval) { > > + netdev_free_custom_stats_counters(&ps.custom_stats); > > return retval != EOF ? retval : 0; > > } > > ofputil_format_port_stats(string, &ps, port_map); > > -- > > 2.7.4 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, Sep 13, 2018 at 06:19:55AM -0700, Yifeng Sun wrote: > When ofputil_decode_port_stats returns error, it is possible that > custom_stats_counters is valid and still need freed. > > The fuzz report link is > https://oss-fuzz.com/testcase?key=5739356233400320 > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> Thank you for fixing this! For future fixes, would you mind using a link to the chromium.org bug tracker in Reported-at:, e.g.: Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9966 That makes it a lot easier to see what patch fixed what reported bug. Thanks, Ben.
Hi Ben, Got it, thanks for telling me. Best, Yifeng On Tue, Sep 18, 2018 at 1:35 AM Ben Pfaff <blp@ovn.org> wrote: > On Thu, Sep 13, 2018 at 06:19:55AM -0700, Yifeng Sun wrote: > > When ofputil_decode_port_stats returns error, it is possible that > > custom_stats_counters is valid and still need freed. > > > > The fuzz report link is > > https://oss-fuzz.com/testcase?key=5739356233400320 > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > Thank you for fixing this! > > For future fixes, would you mind using a link to the chromium.org bug > tracker in Reported-at:, e.g.: > > Reported-at: > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9966 > > That makes it a lot easier to see what patch fixed what reported bug. > > Thanks, > > Ben. >
diff --git a/lib/ofp-print.c b/lib/ofp-print.c index e05a969a82b0..37d7b8b98c55 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -558,6 +558,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, retval = ofputil_decode_port_stats(&ps, &b); if (retval) { + netdev_free_custom_stats_counters(&ps.custom_stats); return retval != EOF ? retval : 0; } ofputil_format_port_stats(string, &ps, port_map);
When ofputil_decode_port_stats returns error, it is possible that custom_stats_counters is valid and still need freed. The fuzz report link is https://oss-fuzz.com/testcase?key=5739356233400320 Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- lib/ofp-print.c | 1 + 1 file changed, 1 insertion(+)