On 6/27/17, 3:00 PM, "Joe Stringer" <[email protected]> wrote:
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.
I looked and noticed that the conn copy in nat_select_range_tuple() was never
needed there; I moved it to conn_not_found(). I agree it was not clear.
* 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.
In the error case, I added a memset zero (with comment) of the
conn_for_un_nat_copy structure to document that the structure will
not be further used.
In the non-error case, those pointers are presently explicitly nulled.
conn_for_un_nat_copy->nat_info = NULL;
conn_for_un_nat_copy->alg = NULL;
I think this makes it clear that conn_for_un_nat_copy is not the owner
of these 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
