| Message ID | 1578955945-19812-1-git-send-email-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/3] ovn-controller.c: Fix possible NULL pointer dereference. | expand |
Hi Han. This patch introduces a small behavior change in the case where cfg is NULL. In the earlier version, we would always set the probe interval on the southbound database. With this version of the patch, we exit early when cfg is NULL and do not set the probe interval. On 1/13/20 5:52 PM, Han Zhou wrote: > In function update_sb_db(), it tries to access cfg->external_ids > outside of the "if (cfg)" block. This patch fixes it. > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > controller/ovn-controller.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 17744d4..3d843f7 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -431,12 +431,12 @@ static void > update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > { > const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); > + if (!cfg) { > + return; > + } > > /* Set remote based on user configuration. */ > - const char *remote = NULL; > - if (cfg) { > - remote = smap_get(&cfg->external_ids, "ovn-remote"); > - } > + const char *remote = smap_get(&cfg->external_ids, "ovn-remote"); > ovsdb_idl_set_remote(ovnsb_idl, remote, true); > > /* Set probe interval, based on user configuration and the remote. */ >
Hi Mark,
Thanks for the review!
In fact, if cfg was NULL, this function would never have a chance to set
probe interval, because before setting the interval it would have already
crashed, at this line:
int interval = smap_get_int(&cfg->external_ids,
So, the behavior change of this patch is only avoiding the crash.
Thanks,
Han
On Tue, Jan 14, 2020 at 1:24 PM Mark Michelson <mmichels@redhat.com> wrote:
>
> Hi Han.
>
> This patch introduces a small behavior change in the case where cfg is
> NULL. In the earlier version, we would always set the probe interval on
> the southbound database. With this version of the patch, we exit early
> when cfg is NULL and do not set the probe interval.
>
> On 1/13/20 5:52 PM, Han Zhou wrote:
> > In function update_sb_db(), it tries to access cfg->external_ids
> > outside of the "if (cfg)" block. This patch fixes it.
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > ---
> > controller/ovn-controller.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 17744d4..3d843f7 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -431,12 +431,12 @@ static void
> > update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl)
> > {
> > const struct ovsrec_open_vswitch *cfg =
ovsrec_open_vswitch_first(ovs_idl);
> > + if (!cfg) {
> > + return;
> > + }
> >
> > /* Set remote based on user configuration. */
> > - const char *remote = NULL;
> > - if (cfg) {
> > - remote = smap_get(&cfg->external_ids, "ovn-remote");
> > - }
> > + const char *remote = smap_get(&cfg->external_ids, "ovn-remote");
> > ovsdb_idl_set_remote(ovnsb_idl, remote, true);
> >
> > /* Set probe interval, based on user configuration and the
remote. */
> >
>
On Wed, Jan 15, 2020 at 3:51 AM Han Zhou <hzhou@ovn.org> wrote: > > Hi Mark, > > Thanks for the review! > In fact, if cfg was NULL, this function would never have a chance to set > probe interval, because before setting the interval it would have already > crashed, at this line: > int interval = smap_get_int(&cfg->external_ids, > > So, the behavior change of this patch is only avoiding the crash. > > Thanks, > Han > > On Tue, Jan 14, 2020 at 1:24 PM Mark Michelson <mmichels@redhat.com> wrote: > > > > Hi Han. > > > > This patch introduces a small behavior change in the case where cfg is > > NULL. In the earlier version, we would always set the probe interval on > > the southbound database. With this version of the patch, we exit early > > when cfg is NULL and do not set the probe interval. > > > > On 1/13/20 5:52 PM, Han Zhou wrote: > > > In function update_sb_db(), it tries to access cfg->external_ids > > > outside of the "if (cfg)" block. This patch fixes it. > > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> Acked-by: Numan Siddique <numans@ovn.org> Numan > > > --- > > > controller/ovn-controller.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 17744d4..3d843f7 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -431,12 +431,12 @@ static void > > > update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > > > { > > > const struct ovsrec_open_vswitch *cfg = > ovsrec_open_vswitch_first(ovs_idl); > > > + if (!cfg) { > > > + return; > > > + } > > > > > > /* Set remote based on user configuration. */ > > > - const char *remote = NULL; > > > - if (cfg) { > > > - remote = smap_get(&cfg->external_ids, "ovn-remote"); > > > - } > > > + const char *remote = smap_get(&cfg->external_ids, "ovn-remote"); > > > ovsdb_idl_set_remote(ovnsb_idl, remote, true); > > > > > > /* Set probe interval, based on user configuration and the > remote. */ > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jan 17, 2020 at 4:45 AM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Jan 15, 2020 at 3:51 AM Han Zhou <hzhou@ovn.org> wrote: > > > > Hi Mark, > > > > Thanks for the review! > > In fact, if cfg was NULL, this function would never have a chance to set > > probe interval, because before setting the interval it would have already > > crashed, at this line: > > int interval = smap_get_int(&cfg->external_ids, > > > > So, the behavior change of this patch is only avoiding the crash. > > > > Thanks, > > Han > > > > On Tue, Jan 14, 2020 at 1:24 PM Mark Michelson <mmichels@redhat.com> wrote: > > > > > > Hi Han. > > > > > > This patch introduces a small behavior change in the case where cfg is > > > NULL. In the earlier version, we would always set the probe interval on > > > the southbound database. With this version of the patch, we exit early > > > when cfg is NULL and do not set the probe interval. > > > > > > On 1/13/20 5:52 PM, Han Zhou wrote: > > > > In function update_sb_db(), it tries to access cfg->external_ids > > > > outside of the "if (cfg)" block. This patch fixes it. > > > > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > Acked-by: Numan Siddique <numans@ovn.org> > > Numan > Thanks for the review. I applied this to master.
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 17744d4..3d843f7 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -431,12 +431,12 @@ static void update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) { const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); + if (!cfg) { + return; + } /* Set remote based on user configuration. */ - const char *remote = NULL; - if (cfg) { - remote = smap_get(&cfg->external_ids, "ovn-remote"); - } + const char *remote = smap_get(&cfg->external_ids, "ovn-remote"); ovsdb_idl_set_remote(ovnsb_idl, remote, true); /* Set probe interval, based on user configuration and the remote. */
In function update_sb_db(), it tries to access cfg->external_ids outside of the "if (cfg)" block. This patch fixes it. Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/ovn-controller.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)