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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
