On 2/4/19, 12:24 PM, "Ben Pfaff" <[email protected]> wrote:
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.
Hard to disprove the possibility in all cases.
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.
Since the goal of the patch is to play it safe in all possible cases, memcpy is
the clear choice.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
