On 13 Jun 2023, at 20:34, 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.
Thanks for the changes, some comments below.
Cheers,
Eelco
> 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..b15758ae1 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;
Add a new line here.
> + if (agent->allocFn) {
> + alloc = (*agent->allocFn)(agent->magic, agent, bytes);
> + ovs_assert(alloc);
> + memset(alloc, 0, bytes);
> + } else {
> + alloc = SFL_ALLOC(bytes);
> + ovs_assert(alloc);
Assert should not be needed as xzalloc will do this.
> + }
> + 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 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;
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
