On 8/24/23 11:20, Eelco Chaudron wrote: > > > On 3 Aug 2023, at 18:19, James Raphael Tiovalen wrote: > >> This commit adds zero-initializations by changing `SFL_ALLOC` from >> `malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`, >> initializing a `pollfd` struct variable with zeroes, and changing some >> calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks >> or undefined behavior from potentially uninitialized variables. >> >> Some variables would always be initialized by either the code flow or >> the compiler. Thus, some of the associated Coverity reports might be >> false positives. That said, it is still considered best practice to >> zero-initialize variables upfront just in case to ensure the overall >> resilience and security of OVS, as long as they do not impact >> performance-critical code. As a bonus, it would also make static >> analyzer tools, such as Coverity, happy. > > Changes look good, however, one small nit, as you need to send a new rev > anyway. > > Cheers, > > Eelco > >> Signed-off-by: James Raphael Tiovalen <[email protected]> >> Reviewed-by: Simon Horman <[email protected]> >> --- >> lib/latch-unix.c | 2 +- >> lib/sflow_agent.c | 12 ++++++++++-- >> lib/sflow_api.h | 2 +- >> utilities/ovs-vsctl.c | 5 ++--- >> 4 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/lib/latch-unix.c b/lib/latch-unix.c >> index f4d10c39a..c62bb024b 100644 >> --- a/lib/latch-unix.c >> +++ b/lib/latch-unix.c >> @@ -71,7 +71,7 @@ latch_set(struct latch *latch) >> bool >> latch_is_set(const struct latch *latch) >> { >> - struct pollfd pfd; >> + struct pollfd pfd = {0}; >> int retval; >> >> pfd.fd = latch->fds[0]; >> diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c >> index c95f654a5..743774a27 100644 >> --- a/lib/sflow_agent.c >> +++ b/lib/sflow_agent.c >> @@ -510,8 +510,16 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, >> char *msg) >> >> static void * sflAlloc(SFLAgent *agent, size_t bytes) >> { >> - if(agent->allocFn) return (*agent->allocFn)(agent->magic, agent, bytes); >> - else return SFL_ALLOC(bytes); >> + void *alloc; >> + >> + if (agent->allocFn) { >> + alloc = (*agent->allocFn)(agent->magic, agent, bytes); >> + ovs_assert(alloc); >> + memset(alloc, 0, bytes); >> + } else { >> + alloc = SFL_ALLOC(bytes); >> + } >> + return alloc; >> } >> >> static void sflFree(SFLAgent *agent, void *obj) >> diff --git a/lib/sflow_api.h b/lib/sflow_api.h >> index a0530b37a..eb23e2acd 100644 >> --- a/lib/sflow_api.h >> +++ b/lib/sflow_api.h >> @@ -337,7 +337,7 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, >> char *msg); >> >> u_int32_t sfl_receiver_samplePacketsSent(SFLReceiver *receiver); >> >> -#define SFL_ALLOC malloc >> +#define SFL_ALLOC xzalloc >> #define SFL_FREE free >> >> #endif /* SFLOW_API_H */ >> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c >> index 62b512302..f55c2965a 100644 >> --- a/utilities/ovs-vsctl.c >> +++ b/utilities/ovs-vsctl.c >> @@ -575,7 +575,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx, >> struct ovsrec_bridge *br_cfg, const char *name, >> struct vsctl_bridge *parent, int vlan) >> { >> - struct vsctl_bridge *br = xmalloc(sizeof *br); >> + struct vsctl_bridge *br = xzalloc(sizeof *br); > > Add a new line here.
Thanks, James, Simon and Eelco! I added some empty lines and applied this one patch. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
