Re: [PATCH] conntrack: fix nfct_clone with certain attribute data types

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Nov 19, 2012 at 10:39:55PM +0100, Florian Westphal wrote:
> some attributes are pointers to malloc'd objects.  Simply
> copying the pointer results in use-after free
> when the original or the clone is destroyed.
> 
> Also, add test case for cloned objects to ensure that
> ct = nfct_new();
> ct2 = nfct_clone(ct);
> nfct_destroy(ct);
> nfct_destroy(ct2);
> 
> won't crash due to double-frees.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  When working on the connlabel stuff I noticed that nfct_clone does a
>  plain memcpy.  Afaics nfct_clone has returned a shallow copy for ages.
>  But documentation implies that it really should do a deep copy.
> 
>  Pablo, could you please double-check?  Thanks!
> 
>  qa/test_api.c       |    6 ++++++
>  src/conntrack/api.c |    2 +-
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/qa/test_api.c b/qa/test_api.c
> index d5f95e9..fa325c8 100644
> --- a/qa/test_api.c
> +++ b/qa/test_api.c
> @@ -2,6 +2,7 @@
>   * Run this after adding a new attribute to the nf_conntrack object
>   */
>  
> +#include <assert.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -199,6 +200,10 @@ int main(void)
>  		eval_sigterm(status);
>  	}
>  
> +	ct2 = nfct_clone(ct);
> +	assert(ct2);
> +	nfct_destroy(ct2);
> +
>  	ct2 = nfct_new();
>  	if (!ct2) {
>  		perror("nfct_new");
> @@ -264,6 +269,7 @@ int main(void)
>  	}
>  
>  	nfct_destroy(ct2);
> +	printf("== destroy cloned ct entry ==\n");
>  	nfct_destroy(ct);
>  	nfct_destroy(tmp);
>  	nfexp_destroy(exp);
> diff --git a/src/conntrack/api.c b/src/conntrack/api.c
> index 000571f..ebf0d52 100644
> --- a/src/conntrack/api.c
> +++ b/src/conntrack/api.c
> @@ -147,7 +147,7 @@ struct nf_conntrack *nfct_clone(const struct nf_conntrack *ct)
>  
>  	if ((clone = nfct_new()) == NULL)
>  		return NULL;
> -	memcpy(clone, ct, sizeof(*ct));
> +	nfct_copy(clone, ct, NFCT_CP_ALL);

That seems safe to me.

Still I think NFCT_CP_OVERRIDE is faster. I added that flag way after
to improve a bit the copying time.

It will require also to add the connlabel support to it. See
__copy_fast.

I need to check this again but I think we can remove
src/conntrack/copy.c and make NFCT_CP_ALL behave just like
NFCT_CP_OVERRIDE.