[ovs-dev] ofp-print: Fix a memory leak reported by fuzz

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
Related show

Commit Message

Yifeng Sun Sept. 13, 2018, 1:19 p.m.
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(+)

Comments

Justin Pettit Sept. 14, 2018, 8:14 p.m. | #1
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
Yifeng Sun Sept. 14, 2018, 8:24 p.m. | #2
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
>
>
Ben Pfaff Sept. 18, 2018, 8:35 a.m. | #3
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.
Yifeng Sun Sept. 18, 2018, 4:51 p.m. | #4
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.
>

Patch

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);