[SSSD] [PATCHES] dyndns: Fix talloc hierarchy of "struct sss_iface_addr"

Jakub Hrozek jhrozek at redhat.com
Thu Aug 21 16:58:04 UTC 2014


On Thu, Aug 21, 2014 at 10:25:08AM +0200, Lukas Slebodnik wrote:
> ehlo
> 
> 0001-dyndns_test-Use-right-socket-length-of-for-IPv4-addr.patch
>     not very important patch.
> 
> 0002-responder-get-domains-tests-fix-checking-of-leaks.patch
> 0003-test_dyndns-Use-different-talloc-context-in-wrapped-.patch
>     previous two patches are necessary for 4th patch
> 
> 0004-TESTS-leak_check-functions-shouldn-t-be-called-with-.patch
>     Wit this patch it will be possible to cach mem leak in 6th patch
> 
> 0005-dyndns-Fix-talloc-hierarchy-of-struct-sss_iface_addr.patch
>     this simpepatch fix use after free problem in ticket #2405
> 
> 0006-test_dyndns-sss_iface_addr_list_get-can-return-more-.patch
>     test for previous patch. You can try to revert 5th patch
>     and you will see memory leak (not all data were moved to appropriate
>             talloc context)
> 
> Leak report for ../sssd/src/tests/cmocka/test_dyndns.c:247:
> full talloc report on 'struct dyndns_test_ctx' (total     80 bytes in   3 blocks)
>     struct sss_iface_addr          contains     40 bytes in   2 blocks (ref 0) 0x1469c20
>         ../sssd/src/providers/dp_dyndns.c:210 contains     16 bytes in   1 blocks (ref 0) 0x1469990
> check_leaks_pop(dyndns_test_ctx) == true
> ../sssd/src/tests/cmocka/test_dyndns.c:247: error: Failure!
> [  FAILED  ] dyndns_test_get_multi_ifaddr
> 
> 0007-SDAP-free-subrequest-in-sdap_dyndns_update_addrs_don.patch
>     subrequest can be freed after fixing talloc hierarchy in 5th patch.
> 
> How to test.
> two IP addresses shoudl be used in ptr update. You can use valgrind to see use
> after free error.
> 
> The other option is to export env variable TALLOC_FREE_FILL=255 and sssd_be
> will crash in function resolv_get_string_ptr_address
> 
> LS

> From c1dcfbf48ad07d3ac34f2085272e64666a0b4239 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Wed, 20 Aug 2014 17:32:04 +0200
> Subject: [PATCH 1/7] dyndns_test: Use right socket length of for IPv4 address.
> 
> man inet_ntop says:
>   The caller specifies the number of bytes available in this buffer
>   in the argument size.
> 
>    AF_INET
>           src points to a struct in_addr (in network byte order) which is
>           converted to an IPv4 network address in the dotted-decimal
>           format, "ddd.ddd.ddd.ddd". The buffer dst must be at least
>           INET_ADDRSTRLEN bytes long.
> ---
>  src/tests/cmocka/test_dyndns.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c
> index b0e8df81bc079efa8ebd8ae45603921261330c9b..b7e45a19db5dd9e85260029fe81e46aa18c1c8ed 100644
> --- a/src/tests/cmocka/test_dyndns.c
> +++ b/src/tests/cmocka/test_dyndns.c
> @@ -194,7 +194,7 @@ void dyndns_test_get_ifaddr(void **state)
>  
>      assert_non_null(inet_ntop(AF_INET,
>                                &((struct sockaddr_in *) addrlist->addr)->sin_addr,
> -                              straddr, INET6_ADDRSTRLEN));
> +                              straddr, INET_ADDRSTRLEN));

Does it make sense to also use INET_ADDRSTRLEN+1 for the straddr length,
too?

>      assert_string_equal(straddr, "192.168.0.1");
>  
>      talloc_free(addrlist);
> -- 
> 2.1.0
> 

> From 54dc1a7132aa71438e0e31ed78a23d0925621a89 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Wed, 20 Aug 2014 19:21:40 +0200
> Subject: [PATCH 2/7] responder-get-domains-tests: fix checking of leaks

ACK

> From 8fef8d5da303a857d9eb689539d1c8ec898fb0c5 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Wed, 20 Aug 2014 19:09:59 +0200
> Subject: [PATCH 3/7] test_dyndns: Use different talloc context in wrapped
>  functions.

ACK

> From 47315445ed15bbdb6ce90555fd37344c12174aad Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Wed, 20 Aug 2014 19:06:48 +0200
> Subject: [PATCH 4/7] TESTS: leak_check functions shouldn't be called with NULL
>  context

ACK

> From 287a672045cb146019ddf83ce2305e5d872c9341 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Wed, 20 Aug 2014 17:04:34 +0200
> Subject: [PATCH 5/7] dyndns: Fix talloc hierarchy of "struct sss_iface_addr"

ACK, nice catch.

> From 36f3308dd3041b48b18769a55ce4040c97bd3cbb Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Wed, 20 Aug 2014 20:42:27 +0200
> Subject: [PATCH 6/7] test_dyndns: sss_iface_addr_list_get can return more
>  values

ACK. Thanks for writing unit tests!


> From 22a5c9304f649f03f8fdb4e3e672f5fe59764f40 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Thu, 21 Aug 2014 09:11:38 +0200
> Subject: [PATCH 7/7] SDAP: free subrequest in sdap_dyndns_update_addrs_done

ACK. The subreq would get freed later by a parent, so the leak is not
critical, but it's the right thing to do.



More information about the sssd-devel mailing list