James Raphael Tiovalen <[email protected]> writes: > 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. > > Signed-off-by: James Raphael Tiovalen <[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); > br->br_cfg = br_cfg; > br->name = xstrdup(name); > ovs_list_init(&br->ports); > @@ -659,7 +659,7 @@ static struct vsctl_port * > add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge > *parent, > struct ovsrec_port *port_cfg) > { > - struct vsctl_port *port; > + struct vsctl_port *port = xzalloc(sizeof *port);
Not sure why we moved the assignment to here? I would prefer it be left in-place and just change the xmalloc to xzalloc. That can probably be changed on apply if you agree, and the maintainer applying it agrees as well. Otherwise, Acked-by: Aaron Conole <[email protected]> > if (port_cfg->tag > && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) { > @@ -671,7 +671,6 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct > vsctl_bridge *parent, > } > } > > - port = xmalloc(sizeof *port); > ovs_list_push_back(&parent->ports, &port->ports_node); > ovs_list_init(&port->ifaces); > port->port_cfg = port_cfg; _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
