Re: [nft PATCH] datatype: fix leak and cleanup reference counting for struct datatype

[Thomas Haller <thaller@redhat.com>]

Hi Pablo,

> > @@ -3806,37 +3833,43 @@ static int stmt_evaluate_nat_map(struct
> > eval_ctx *ctx, struct stmt *stmt)
> >  
> >         datatype_set(data, dtype);
> 
> Note here above, dtype is set to data expression, then...
> 
> >  
> > -       if (expr_ops(data)->type != EXPR_CONCAT)
> > -               return __stmt_evaluate_arg(ctx, stmt, dtype, dtype-
> > >size,
> > +       if (expr_ops(data)->type != EXPR_CONCAT) {
> > +               r = __stmt_evaluate_arg(ctx, stmt, dtype, dtype-
> > >size,
> >                                            BYTEORDER_BIG_ENDIAN,
> >                                            &stmt->nat.addr);
> > +               goto out;
> 
> ... this goto is not needed anymore? dtype has been already set to
> data.
> So this patch can be simplified. Same things for goto below in the
> scope of this function.

After datatype_set(data, dtype) the reference to "dtype" must still be
released. The simple way is

   datatype_set(data, dtype);
   datatype_free(dtype);
   dtype = NULL;

and make sure to not use "dtype" afterwards, but data->dtype.

But there are already "goto out" earlier. So this change of cleanup-
style halfway through is error prone. We could also drop the other
"goto out" and explicitly implement cleanup at the multiple exit points
of the function. But that's also error prone and redundant.

The safe thing is to be very clear which variable holds a reference
(dtype), and always+only release it at the end (goto out).

The real solution to all of this would be __attribute__((cleanup)) and
let the compiler help. Unless that is used, a consistent `goto out` is
the cleanest solution.

It's about consistently doing "goto out", and not that you could rework
one "goto-out" with something else to safe a few lines of code. Once we
do "goto out", there is no need trying to release references early or
do anything smart with transferring ownership.


> > @@ -978,7 +983,8 @@ struct set *netlink_delinearize_set(struct
> > netlink_ctx *ctx,
> >         if (keytype == NULL) {
> >                 netlink_io_error(ctx, NULL, "Unknown data type in
> > set key %u",
> >                                  key);
> > -               return NULL;
> > +               set = NULL;
> > +               goto out;
> 
> Why this goto out? Not really needed.

No, it's not needed for technical reasons. It's there for consistency
regarding the release of the pointers. Anyway, I drop this.

I will later propose a patch using __attribute__((cleanup)) for
comparison.


> 
> >         }
> >  
> >         flags = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
> > @@ -991,8 +997,8 @@ struct set *netlink_delinearize_set(struct
> > netlink_ctx *ctx,
> >                         netlink_io_error(ctx, NULL,
> >                                          "Unknown data type in set
> > key %u",
> >                                          data);
> > -                       datatype_free(keytype);
> > -                       return NULL;
> > +                       set = NULL;
> > +                       goto out;
> >                 }
> >         }
> >  
> > @@ -1030,19 +1036,18 @@ struct set *netlink_delinearize_set(struct
> > netlink_ctx *ctx,
> >         if (datatype) {
> 
> Move this code under this if (datatype) branch into function in a
> preparation patch.
> 
> Please, call it:
> 
>         netlink_delinearize_set_typeof()
> 
> or pick a better name if you like so there is no need for dtype2.
> 
> It will help clean up this chunk that you are passing by here.

Not sure how to do that. For one, the if-block is only used at one
place. So the function isn't really reusable (or where can I also reuse
it)?. Also, it uses quite many local variables. If all those become
function parameters, it's confusing. If I move only a subset of the
block to the new function, it's not clear how it simplifies anything. I
think there is not a sufficiently isolated functionality, that warrants
a function.

netlink_delinearize_set() is large and allocates up to 4 datattypes
that we need to release (datatype, keytype, dtype2, dtype). With the
patch, the pattern is very simple, that those 4 variables get only
assigned at one place, and always released only at the end (goto out).
Moving the block to a separate function, at most safes dtype2 but
doesn't remove the general need for such release-at-end. If I really
wanted, I could move dtype2 inside the if-block. But that doesn't seem
useful, given there is a consistent RAII-like cleanup mechanism in
place.



Other points will be fixed in v2.

Thomas