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@xxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
