[ovs-dev] [patch v3] conntrack: Fix possibly uninitialized memory.

Ben Pfaff blp at ovn.org
Mon Feb 4 19:11:20 UTC 2019


On Sun, Feb 03, 2019 at 02:15:27PM -0800, Darrell Ball wrote:
> There are a few cases where padding may be undefined according to
> the C standard.  Practically, it seems implementations don't have issue,
> but it is better to be safe. The code paths modified are not hot ones.
> 
> Found by inspection.
> 
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> ---
> 
> v3: Removed an unnecessary change and limited scope of 2 others.
> v2: Found another one; will double check for others later.
> 
>  lib/conntrack.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e1f4041..4c8d71b 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -748,6 +748,7 @@ conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now)
>  {
>      struct conn_lookup_ctx ctx;
>      ctx.conn = NULL;
> +    memset(&ctx.key, 0, sizeof ctx.key);
>      ctx.key = *key;
>      ctx.hash = conn_key_hash(key, ct->hash_basis);
>      unsigned bucket = hash_to_bucket(ctx.hash);

We are talking about technical aspects of the C standard here.  While
it's a somewhat petty distinction, padding bytes take unspecified
values, not undefined ones.

One can certainly assign values to the padding bytes with memset (or
otherwise using character types), but there's no guarantee (as far as I
know) that struct assignment doesn't overwrite the padding bytes with
unspecified values.  C11 6.2.6.1#11 essentially says that;

    When a value is stored in an object of structure or union type,
    including in a member object, the bytes of the object representation
    that correspond to any padding bytes take unspecified values.51)

    51) Thus, for example, structure assignment need not copy any
    padding bits.

So I don't think there is value, standards-wise, to putting a memset
here.  I don't know whether there is value, implementation-wise.  Do you
know of a reason to believe it?

If we have a "delicate" type like this, then maybe we should use
memcpy() to copy it.  (And be sure that we are somehow initializing
those padding bytes in the source of the memcpy().)


More information about the dev mailing list