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