On Wed, May 03, 2023 at 02:30:27AM +0800, 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.
>
> Signed-off-by: James Raphael Tiovalen <jamestio...@gmail.com>
> ---
> lib/latch-unix.c | 2 +-
> lib/sflow_agent.c | 11 +++++++++--
> lib/sflow_api.h | 2 +-
> utilities/ovs-vsctl.c | 5 ++---
> 4 files changed, 13 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..162e3c3f4 100644
> --- a/lib/sflow_agent.c
> +++ b/lib/sflow_agent.c
> @@ -510,8 +510,15 @@ 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);
> + memset(alloc, 0, bytes);
> + } else {
> + alloc = SFL_ALLOC(bytes);
> + }
> + ovs_assert(alloc);
> + return alloc;
> }
On question about this.
memset() is passed alloc before the call to ovs_assert().
>From the POV of the safety you are implementing, is that ok?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
