On Fri, May 10, 2024 at 1:34 PM Ilya Maximets <[email protected]> wrote:
> On 5/10/24 12:57, Ales Musil wrote:
> > The bitmap used in the update_ct_zones was uninitialized, and it could
> > contain any value besides 0. Use the bitmap_allocate() function instead,
> > to allocate the bitmap in heap rather than stack, the allocate makes sure
> > that the memory is properly zeroed.
> > This was caught by valgrind:
> >
> > Conditional jump or move depends on uninitialised value(s)
> > at 0x44074B: update_ct_zones (ovn-controller.c:812)
> > by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579)
> > by 0x468BB7: engine_recompute (inc-proc-eng.c:415)
> > by 0x46954C: engine_compute (inc-proc-eng.c:454)
> > by 0x46954C: engine_run_node (inc-proc-eng.c:503)
> > by 0x46954C: engine_run (inc-proc-eng.c:528)
> > by 0x40AE9D: main (ovn-controller.c:5776)
> > Uninitialised value was created by a stack allocation
> > at 0x440313: update_ct_zones (ovn-controller.c:747)
> >
> > Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a
> gateway router.")
> > Signed-off-by: Ales Musil <[email protected]>
> > ---
> > v2: Use bitmap_allocate() instead of array on stack.
> > ---
> > controller/ovn-controller.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 453dc62fd..8ee2da2fd 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports,
> > const char *user;
> > struct sset all_users = SSET_INITIALIZER(&all_users);
> > struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones);
> > - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)];
> > + unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES);
> > struct simap unreq_snat_zones =
> SIMAP_INITIALIZER(&unreq_snat_zones);
> >
> > const char *local_lport;
> > @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports,
> > simap_destroy(&req_snat_zones);
> > simap_destroy(&unreq_snat_zones);
> > sset_destroy(&all_users);
> > + free(unreq_snat_zones_map);
> > }
> >
> > static void
>
> Thanks, Ales. This change LGTM.
>
> Though I'm a bit surprised asan didn't catch this. Is this code not
> covered by tests?
>
It should be covered indirectly by a lot of tests, but there are some that
target this code specifically e.g. "resolve CT zone conflicts from ovsdb".
>
> Best regards, Ilya Maximets.
>
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected]
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
