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 <[email protected]>
> ---
> 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;
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
