On Mon, Feb 04, 2019 at 07:52:18PM +0000, Darrell Ball wrote: > > > On 2/4/19, 11:15 AM, "[email protected] on behalf of Ben > Pfaff" <[email protected] on behalf of [email protected]> 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 <[email protected]> > > --- > > > > 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.
The standard does not say much in this area. However, if the spec writers meant for there to be a limited number of possibilities, then the logical way to express that would have been to explain those possibilities, or to at least term this implementation-defined behavior instead of leaving it unspecified. I don't know of any constraints that prevent possibility #3. The "obvious" way that it would happen is a compiler that generates a partial-word load and then a full-word write, so that data previously in the register before the partial load "leaks" into the full-word write. I don't know that any compiler does that--but I also don't know that compilers are written to avoid it. It's the sort of thing where you don't find out until you troubleshoot a really nasty bug that only appears with GCC x.y.z. > 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 the goal is to avoid poorly defined or specified behavior, then I'd argue for memcpy. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
