On 5/10/24 13:44, Ales Musil wrote: > > > On Fri, May 10, 2024 at 1:34 PM Ilya Maximets <[email protected] > <mailto:[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] <mailto:[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".
I'd expect asan to catch use of uninitialized stack memory. Weird. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
