| Message ID | 20230415152155.762025-3-jamestiotio@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | treewide: Fix multiple Coverity defects | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings James Raphael Tiovalen, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line has non-spaces leading whitespace #57 FILE: lib/sflow_agent.c:207: ovs_assert(newsm); WARNING: Line has non-spaces leading whitespace #65 FILE: lib/sflow_agent.c:247: ovs_assert(newpl); Lines checked: 115, Warnings: 2, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Sat, Apr 15, 2023 at 11:21:49PM +0800, James Raphael Tiovalen wrote: > This commit adds zero-initializations by changing `SFL_ALLOC` from > `malloc` to `xzalloc`, 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 <jamestiotio@gmail.com> > --- > lib/latch-unix.c | 2 +- > lib/sflow_agent.c | 3 +++ > lib/sflow_api.h | 2 +- > utilities/ovs-vsctl.c | 5 ++--- > 4 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/lib/latch-unix.c b/lib/latch-unix.c > index f4d10c39a..262f111e4 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, 0, 0}; Perhaps it's just me, but I'd have written this as: 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..0bb5ae7e9 100644 > --- a/lib/sflow_agent.c > +++ b/lib/sflow_agent.c > @@ -153,6 +153,7 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) > SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) > { > SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); > + ovs_assert(rcv); I was wondering if we could move the assert inside sflAlloc. And also have sflAlloc zero the memory, perhaps only in the callback case to allow xzalloc to do it's thing. > sfl_receiver_init(rcv, agent); > /* add to end of list - to preserve the receiver index numbers for existing receivers */ > { > @@ -203,6 +204,7 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) > > { > SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); > + ovs_assert(newsm); > sfl_sampler_init(newsm, agent, pdsi); sfl_sampler_init zero's newsm. If it is always zero, perhaps that memset can be removed from sfl_sampler_init(). Avoiding extra work. Or is that too risky in your opinion? > if(prev) prev->nxt = newsm; > else agent->samplers = newsm; > @@ -242,6 +244,7 @@ SFLPoller *sfl_agent_addPoller(SFLAgent *agent, > /* either we found the insert point, or reached the end of the list... */ > { > SFLPoller *newpl = (SFLPoller *)sflAlloc(agent, sizeof(SFLPoller)); > + ovs_assert(newpl); > sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); > if(prev) prev->nxt = newpl; > else agent->pollers = newpl;
On Fri, Apr 21, 2023 at 6:11 PM Simon Horman <simon.horman@corigine.com> wrote: > > Perhaps it's just me, but I'd have written this as: > > struct pollfd pfd = {0}; > Got it. I will update this in the next patch version. > > I was wondering if we could move the assert inside sflAlloc. > And also have sflAlloc zero the memory, perhaps only in the callback > case to allow xzalloc to do it's thing. > Sure thing. I will implement this in the next patch version. > > sfl_sampler_init zero's newsm. > If it is always zero, perhaps that memset can be removed from > sfl_sampler_init(). Avoiding extra work. Or is that too risky in your > opinion? > `sfl_receiver_init()` and `sfl_poller_init()` also zero `rcv` and `newpl` respectively. However, I do not think that we should remove the `memset()`s in the `*_init()` functions as they are also used when the SFLReceiver, SFLSampler, or SFLPoller is reset. I would recommend keeping both. Best regards, James Raphael Tiovalen
diff --git a/lib/latch-unix.c b/lib/latch-unix.c index f4d10c39a..262f111e4 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, 0, 0}; int retval; pfd.fd = latch->fds[0]; diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c index c95f654a5..0bb5ae7e9 100644 --- a/lib/sflow_agent.c +++ b/lib/sflow_agent.c @@ -153,6 +153,7 @@ void sfl_agent_tick(SFLAgent *agent, time_t now) SFLReceiver *sfl_agent_addReceiver(SFLAgent *agent) { SFLReceiver *rcv = (SFLReceiver *)sflAlloc(agent, sizeof(SFLReceiver)); + ovs_assert(rcv); sfl_receiver_init(rcv, agent); /* add to end of list - to preserve the receiver index numbers for existing receivers */ { @@ -203,6 +204,7 @@ SFLSampler *sfl_agent_addSampler(SFLAgent *agent, SFLDataSource_instance *pdsi) { SFLSampler *newsm = (SFLSampler *)sflAlloc(agent, sizeof(SFLSampler)); + ovs_assert(newsm); sfl_sampler_init(newsm, agent, pdsi); if(prev) prev->nxt = newsm; else agent->samplers = newsm; @@ -242,6 +244,7 @@ SFLPoller *sfl_agent_addPoller(SFLAgent *agent, /* either we found the insert point, or reached the end of the list... */ { SFLPoller *newpl = (SFLPoller *)sflAlloc(agent, sizeof(SFLPoller)); + ovs_assert(newpl); sfl_poller_init(newpl, agent, pdsi, magic, getCountersFn); if(prev) prev->nxt = newpl; else agent->pollers = newpl; 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 2f5ac1a26..b3c75f8ba 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); 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;
This commit adds zero-initializations by changing `SFL_ALLOC` from `malloc` to `xzalloc`, 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 <jamestiotio@gmail.com> --- lib/latch-unix.c | 2 +- lib/sflow_agent.c | 3 +++ lib/sflow_api.h | 2 +- utilities/ovs-vsctl.c | 5 ++--- 4 files changed, 7 insertions(+), 5 deletions(-)