Darrell Ball <[email protected]> writes: > 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.
Better to be safe how? What do implementations not have issue with? I'm having trouble understanding this commit message as a rationale for the change. > Signed-off-by: Darrell Ball <[email protected]> > --- > > v2: Found another one; will double check for others later. Were there actual problems observed somewhere? From what I can see, none of these accesses did any kind of bit-wise structure memcmp(). Rather, I see use of named fields when doing comparisons. This tells me that the pad bits should be irrelevant. Probably I missed something. OTOH, I guess you might also be interested in fixing: 'struct ct_addr first_addr = ct_addr;' on line 2181 of conntrack.c and the 'process_ftp_ctl_v6' function. Those do possibly use memcmp() with indeterminate pad bits, if I'm reading them correctly (but just cursory reading). > lib/conntrack.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index e1f4041..e7033a8 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -747,6 +747,7 @@ static struct conn * > conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now) > { > struct conn_lookup_ctx ctx; > + memset(&ctx, 0, sizeof ctx); > ctx.conn = NULL; > ctx.key = *key; > ctx.hash = conn_key_hash(key, ct->hash_basis); How does this help? Looking at conn_key_hash it seems to only use those defined sections of the struct (ie: it doesn't walk the raw memory, it pulls the fields it needs by assignment). conn_key_cmp also compares named fields. > @@ -896,6 +897,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet > *pkt, > > if (nat_action_info) { > nc->nat_info = xmemdup(nat_action_info, sizeof *nc->nat_info); > + memset(conn_for_un_nat_copy, 0, sizeof *conn_for_un_nat_copy); Was there an observable bug related to the pad bits here? I didn't walk this path deep enough, but in conntrack.c I didn't see any related memcmp() calls. > if (alg_exp) { > if (alg_exp->nat_rpl_dst) { > @@ -934,8 +936,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet > *pkt, > ct_rwlock_unlock(&ct->resources_lock); > } > conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT; > - conn_for_un_nat_copy->nat_info = NULL; > - conn_for_un_nat_copy->alg = NULL; If this was needed here before, isn't it still needed now? There are a interrim assignments from 'nc', and I think that won't be changed by doing a memset() (ie: does this still cause a double free if you drop it?). > nat_packet(pkt, nc, ctx->icmp_related); > } > hmap_insert(&ct->buckets[bucket].connections, &nc->node, ctx->hash); > @@ -1262,6 +1262,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, > ct->hash_basis, > alg_src_ip_wc(ct_alg_ctl)); > if (alg_exp) { > + memset(&alg_exp_entry, 0, sizeof alg_exp_entry); > alg_exp_entry = *alg_exp; > alg_exp = &alg_exp_entry; > } > @@ -2024,7 +2025,9 @@ conn_key_hash(const struct conn_key *key, uint32_t > basis) > static void > conn_key_reverse(struct conn_key *key) > { > - struct ct_endpoint tmp = key->src; > + struct ct_endpoint tmp; > + memset(&tmp, 0, sizeof tmp); > + tmp = key->src; I don't understand the reason to clear the pad bits of tmp here - you only use the named fields and they should be of the same size (and if not, the compiler should be performing integer widening / truncation properly). Did I miss something? > key->src = key->dst; > key->dst = tmp; > } > @@ -2614,7 +2617,9 @@ static struct alg_exp_node * > expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key, > uint32_t basis, bool src_ip_wc) > { > - struct conn_key check_key = *key; > + struct conn_key check_key; > + memset(&check_key, 0, sizeof check_key); > + check_key = *key; > check_key.src.port = ALG_WC_SRC_PORT; > > if (src_ip_wc) { Again, conn_key_hash and conn_key_cmp should be correct, no? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
