[ovs-dev] [PATCH 01/22] ovn-controller: Fix potential use-after-free in get_core_config().

Ben Pfaff blp at nicira.com
Tue Jul 28 15:32:56 UTC 2015


On Sat, Jul 25, 2015 at 11:04:13PM -0700, Alex Wang wrote:
> On Sun, Jul 19, 2015 at 3:44 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > It's unsafe to hold a pointer to a row in the IDL across calls to
> > ovsdb_idl_run() for that IDL.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  ovn/controller/ovn-controller.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/ovn/controller/ovn-controller.c
> > b/ovn/controller/ovn-controller.c
> > index fda1534..e571bb5 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -95,16 +95,15 @@ get_bridge(struct controller_ctx *ctx, const char
> > *name)
> >  static void
> >  get_core_config(struct controller_ctx *ctx)
> >  {
> > -    const struct ovsrec_open_vswitch *cfg;
> > -
> > -    cfg = ovsrec_open_vswitch_first(ctx->ovs_idl);
> > -    if (!cfg) {
> > -        VLOG_ERR("No Open_vSwitch row defined.");
> > -        ovsdb_idl_destroy(ctx->ovs_idl);
> > -        exit(EXIT_FAILURE);
> > -    }
> > -
> >      while (1) {
> > +        const struct ovsrec_open_vswitch *cfg;
> > +        cfg = ovsrec_open_vswitch_first(ctx->ovs_idl);
> > +        if (!cfg) {
> > +            VLOG_ERR("No Open_vSwitch row defined.");
> > +            ovsdb_idl_destroy(ctx->ovs_idl);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +
> >
> 
> Curious, why don't you call ovsdb_idl_run(ctx->ovs_idl) first in the while
> loop?  seems to me, theoretically, cfg could still get changed after
> ovsdb_idl_run(ctx->ovs_idl).

You're right, that was a dumb "fix".

Here's an incremental that fixes that problem.

I'll fix for v2.

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index e571bb5..bd3ef0d 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -96,6 +96,8 @@ static void
 get_core_config(struct controller_ctx *ctx)
 {
     while (1) {
+        ovsdb_idl_run(ctx->ovs_idl);
+
         const struct ovsrec_open_vswitch *cfg;
         cfg = ovsrec_open_vswitch_first(ctx->ovs_idl);
         if (!cfg) {
@@ -107,8 +109,6 @@ get_core_config(struct controller_ctx *ctx)
         const struct ovsrec_bridge *br_int;
         const char *remote, *system_id, *br_int_name;
 
-        ovsdb_idl_run(ctx->ovs_idl);
-
         br_int_name = smap_get(&cfg->external_ids, "ovn-bridge");
         if (!br_int_name) {
             br_int_name = DEFAULT_BRIDGE_NAME;



More information about the dev mailing list