On 2/4/19, 11:15 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-boun...@openvswitch.org on behalf of b...@ovn.org> wrote:
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 <dlu...@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. That is a very interesting interpretation of the standard. My understanding is that there were only two possibilities 1/ Structure assignment copies the padding, which are always originally initialized 2/ Structure assignment simply skips copying the padding, in which case the destination padding bytes are left as they were b4. The third possibility of structure assignment overwriting the padding with unspecified values is not something I considered, because it seemed pointless to do that. 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? memcpy is certainly the first choice that came to mind I chose memset because: 1/ It is highly optimized and combined with structure copy (also optimized) is potentially faster than memcpy. 2/ memset zero and structure copy are easier to digest compared to memcpy, although there are two lines instead of one. I am certainly fine either way though. 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().) _______________________________________________ dev mailing list d...@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cdball%40vmware.com%7C8aea287cf6c24446a89e08d68ad51118%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636849045150275812&sdata=Zx0550Y62amgeZi7wDTnw144hBYPCLJgp1vRrzSdf%2BY%3D&reserved=0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev