On 27 June 2017 at 13:40, Darrell Ball <[email protected]> wrote: > > > On 6/27/17, 1:20 PM, "Joe Stringer" <[email protected]> wrote: > > On 27 June 2017 at 11:19, Darrell Ball <[email protected]> wrote: > > > > > > On 6/27/17, 11:04 AM, "Joe Stringer" <[email protected]> wrote: > > > > On 27 June 2017 at 10:51, Darrell Ball <[email protected]> wrote: > > > > > > > > > On 6/27/17, 10:47 AM, "Joe Stringer" <[email protected]> wrote: > > > > > > On 27 June 2017 at 10:42, Darrell Ball <[email protected]> > wrote: > > > > > > > > > > > > On 6/27/17, 10:40 AM, "Joe Stringer" <[email protected]> wrote: > > > > > > > > On 26 June 2017 at 18:19, Darrell Ball > <[email protected]> wrote: > > > > > > > > > > > > > > > On 6/26/17, 4:49 PM, "Joe Stringer" <[email protected]> > wrote: > > > > > > > > > > On 26 June 2017 at 13:22, Joe Stringer > <[email protected]> wrote: > > > > > > On 26 June 2017 at 11:45, Darrell Ball > <[email protected]> wrote: > > > > > >> On 6/26/17, 10:22 AM, "Joe Stringer" > <[email protected]> wrote: > > > > > >> On 23 June 2017 at 18:57, Darrell Ball > <[email protected]> wrote: > > > > > >> > On 6/23/17, 4:08 PM, > "[email protected] on behalf of Joe Stringer" > <[email protected] on behalf of [email protected]> wrote: > > > > > >> > On 17 June 2017 at 15:53, Darrell > Ball <[email protected]> wrote: > > > > > >> > > @@ -554,34 +681,50 @@ > conn_not_found(struct conntrack *ct, struct dp_packet *pkt, > > > > > >> > > nc->rev_key = nc->key; > > > > > >> > > > conn_key_reverse(&nc->rev_key); > > > > > >> > > > > > > > >> > > + if (helper) { > > > > > >> > > + nc->alg = > xstrdup(helper); > > > > > >> > > + } > > > > > >> > > + > > > > > >> > > + if (alg_exp) { > > > > > >> > > + nc->alg_related = > true; > > > > > >> > > + nc->mark = > alg_exp->master_mark; > > > > > >> > > + nc->label = > alg_exp->master_label; > > > > > >> > > + nc->master_key = > alg_exp->master_key; > > > > > >> > > + } > > > > > >> > > + > > > > > >> > > if (nat_action_info) { > > > > > >> > > nc->nat_info = > xmemdup(nat_action_info, sizeof *nc->nat_info); > > > > > >> > > - > ct_rwlock_wrlock(&ct->nat_resources_lock); > > > > > >> > > - > > > > > >> > > - bool nat_res = > nat_select_range_tuple(ct, nc, > > > > > >> > > - > conn_for_un_nat_copy); > > > > > >> > > > > > > > >> > > - if (!nat_res) { > > > > > >> > > - > free(nc->nat_info); > > > > > >> > > - nc->nat_info = > NULL; > > > > > >> > > - free (nc); > > > > > >> > > - > ct_rwlock_unlock(&ct->nat_resources_lock); > > > > > >> > > - return NULL; > > > > > >> > > - } > > > > > >> > > + if (alg_exp) { > > > > > >> > > + > nc->rev_key.src.addr = alg_nat_repl_addr; > > > > > >> > > + > nc->nat_info->nat_action = NAT_ACTION_DST; > > > > > >> > > + > *conn_for_un_nat_copy = *nc; > > > > > >> > > + } else { > > > > > >> > > + > ct_rwlock_wrlock(&ct->resources_lock); > > > > > >> > > + bool nat_res = > nat_select_range_tuple( > > > > > >> > > + > ct, nc, conn_for_un_nat_copy); > > > > > >> > > + > > > > > >> > > + if (!nat_res) { > > > > > >> > > + > free(nc->nat_info); > > > > > >> > > + nc->nat_info > = NULL; > > > > > >> > > + free (nc); > > > > > >> > > > > > > >> > I think that nc->alg may be leaked > here? any reason it doesn't use > > > > > >> > delete_conn()? > > > > > >> > > > > > > >> > Good > > > > > >> > Yes, alg will leak in this rare error > case and yes, delete_conn() should be used > > > > > >> > here, as everywhere. > > > > > >> > > > > > >> OK. > > > > > >> > > > > > >> > > + > ct_rwlock_unlock(&ct->resources_lock); > > > > > >> > > + return NULL; > > > > > >> > > + } > > > > > >> > > > > > > > >> > > - if > (conn_for_un_nat_copy && > > > > > >> > > - nc->conn_type == > CT_CONN_TYPE_DEFAULT) { > > > > > >> > > *nc = > *conn_for_un_nat_copy; > > > > > >> > > > > > > >> > Perhaps nc->alg and/or nc->nat_info > may be leaked here? > > > > > >> > > > > > > >> > No, the un_nat conn has no such > allocations, so there is nothing to leak. > > > > > >> > > > > > >> I don't mean conn_for_un_nat_copy, I mean > *nc which could have had an > > > > > >> xstrdup()'d 'alg' attached. Won't this > overwrite all fields in 'nc'? > > > > > >> > > > > > >> I see your question now. > > > > > >> No, at this point, the copy gets the same > pointers to the alg string and nat_info. > > > > > >> Only nc needs them and the un_nat copy ptrs > are nulled. > > > > > >> There is only one allocation set. > > > > > > > > > > > > Hmm. Maybe I'm just missing something, let me > walk through it step by > > > > > > step below and let's see where it goes. > > > > > > > > > > > > if (helper) { > > > > > > nc->alg = xstrdup(helper); > > > > > > ^ nc->alg is set > > > > > > } > > > > > > > > > > > > if (alg_exp) { > > > > > > ^ false; do not execute this block > > > > > > nc->alg_related = true; > > > > > > nc->mark = alg_exp->master_mark; > > > > > > nc->label = alg_exp->master_label; > > > > > > nc->master_key = alg_exp->master_key; > > > > > > } > > > > > > > > > > > > if (nat_action_info) { > > > > > > ^ true, execute this part > > > > > > nc->nat_info = > xmemdup(nat_action_info, sizeof *nc->nat_info); > > > > > > > > > > > > if (alg_exp) { > > > > > > ^ false; skip to else > > > > > > nc->rev_key.src.addr = > alg_nat_repl_addr; > > > > > > nc->nat_info->nat_action = > NAT_ACTION_DST; > > > > > > *conn_for_un_nat_copy = *nc; > > > > > > } else { > > > > > > ^ We go through this condition > > > > > > > ct_rwlock_wrlock(&ct->resources_lock); > > > > > > bool nat_res = > nat_select_range_tuple( > > > > > > ct, nc, > conn_for_un_nat_copy); > > > > > > > > > > > > if (!nat_res) { > > > > > > ^ false; do not execute this block > > > > > > free(nc->nat_info); > > > > > > nc->nat_info = NULL; > > > > > > free (nc); > > > > > > > ct_rwlock_unlock(&ct->resources_lock); > > > > > > return NULL; > > > > > > } > > > > > > > > > > > > *nc = *conn_for_un_nat_copy; > > > > > > ^ Now: > > > > > > nc->alg is overwritten by > conn_for_un_nat_copy->alg > > > > > > nc->nat_info is overwritten by > conn_for_un_nat_copy->nat_info > > > > > > > > > > > > We don't free either of these. > > > > > > > > > > As discussed offline, the copy of '*nc' into > '*conn_for_un_nat_copy' > > > > > nested inside nat_select_range_tuple() is very > well hidden. This means > > > > > that the above is not a problem... but what if > (!nat_res) ? Then > > > > > conn_for_un_nat_copy() has a reference to these > alg/nat_info > > > > > parameters which are freed from 'nc' inside that > block, then > > > > > 'conn_for_un_nat_copy' is returned. Could there > be a use-after-free > > > > > then? > > > > > > > > > > Nope, because there is no un_nat conn. > > > > > > > > So you mean that dangling references are returned inside > > > > conn_for_un_nat_copy but they're just not used? > > > > > > > > JTBC, nothing can be used, since there is not even a > connection. > > > > > > So if nat_select_range_tuple() is called, and it runs the > "*nat_conn = > > > *conn;" line which updates conn_for_un_nat_copy, then > > > nat_select_range_tuple() fails to select a tuple and returns > false, > > > then the if (!nat_res) cleanup / return NULL path is called, > then what > > > cleans up the dangling pointers that are now in > conn_for_un_nat_copy? > > > > > > conn_for_un_nat_copy is a local variable in the caller > > > > Which is then potentially passed to another function, > > create_un_nat_conn(), which then does an xmemdup() to propagate > those > > dangling pointers further.. seems like it would be safest to clear > out > > the conn_for_un_nat_copy->alg and ->nat_info pointers before freeing > > 'nc' in that path. > > > > No, as I stated already, since the un_nat creation (and also conn) > never completed > > the conntype is not set and create_un_nat_conn() is not called. > > OK, now I see what you mean. > > I think that clearing these pointers in that error path would make the > code more explicit about the status of one of the parameters modified > by this function, and would limit side-effects. I don't see a negative > to this proposed change. > > Adding redundant clearing of pointers in the error path will not hurt > performance, so there is no problem there.
Right. > It feels a little weird to clear a couple pointers in a case where the whole > connection > context is dead. It feels like painting your house while it is burning down. > Maybe a comment would serve the same purpose. Comments could help to some degree, but I think that my core concerns could be addressed with improvements to the code: * nat_select_range_tuple() sounds like it will select a range. Not only does it select the range, but it does full initialization of conn_for_un_nat_copy as well (via shallow-copy) * conn_not_found() allocates memory, then nat_select_range_tuple() shares pointers to that memory around, and yet the pointers don't share fate when conn_not_found() frees them. This makes it difficult to read the code and balance alloc/free. * nc seems to be the owner of these memory allocations, but since they're propagated into conn_for_un_nat_copy, it's less clear which conn is supposed to own it. * The condition where these dangling pointers are shared up to the caller is not the exact same condition check in the original caller which prevents usage of those dangling pointers. A bunch of the above concerns aren't specific to this patch, they already exist in the upstream codebase and I'm probably just noticing because it's really new code and it's my first real dive into this area. But I do think there are improvements to be made in limiting the scope of potential changes from each function, and localizing things like initialization, memory alloc/free/ownership, etc. into ideally the same function. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
