Hi,
The attached patch is a self-contained change related to https://fedorahosted.org/sssd/ticket/1884. I also wrote code that uses the TTL values in the failover code: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=srvttl
But when creating the unit tests I stumbled upon: https://fedorahosted.org/sssd/ticket/2533 and especially: https://fedorahosted.org/sssd/ticket/2532
Until these two are fixed, I don't think we can merge the TTL patch. Feel free to review the changes in my git branch, though, I don't think they'd change.
On Mon, Dec 15, 2014 at 03:15:37AM +0100, Jakub Hrozek wrote:
Hi,
The attached patch is a self-contained change related to https://fedorahosted.org/sssd/ticket/1884. I also wrote code that uses the TTL values in the failover code: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=srvttl
But when creating the unit tests I stumbled upon: https://fedorahosted.org/sssd/ticket/2533 and especially: https://fedorahosted.org/sssd/ticket/2532
Until these two are fixed, I don't think we can merge the TTL patch. Feel free to review the changes in my git branch, though, I don't think they'd change.
I changed the function itself to return a separate status as return value and fill a pointer with the TTL.
Still waiting for CI to finish, but the amended patch is attached.
On Wed, Jan 14, 2015 at 04:21:03PM +0100, Jakub Hrozek wrote:
Still waiting for CI to finish, but the amended patch is attached.
here we go: http://sssd-ci.duckdns.org/logs/commit/76/ee3ec0f68db9be1bb11ba0d20b852c8dcf...
On 01/14/2015 04:21 PM, Jakub Hrozek wrote:
On Mon, Dec 15, 2014 at 03:15:37AM +0100, Jakub Hrozek wrote:
Hi,
The attached patch is a self-contained change related to https://fedorahosted.org/sssd/ticket/1884. I also wrote code that uses the TTL values in the failover code: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=srvttl
But when creating the unit tests I stumbled upon: https://fedorahosted.org/sssd/ticket/2533 and especially: https://fedorahosted.org/sssd/ticket/2532
Until these two are fixed, I don't think we can merge the TTL patch. Feel free to review the changes in my git branch, though, I don't think they'd change.
I changed the function itself to return a separate status as return value and fill a pointer with the TTL.
Still waiting for CI to finish, but the amended patch is attached.
static bool resolv_get_ttl(const unsigned char *abuf, const int alen, uint32_t *_ttl) { const unsigned char *aptr; int ret; char *name = NULL; long len; uint32_t ttl = 0; uint32_t rr_ttl; unsigned int rr_len; unsigned int ancount; unsigned int i;
/* Read the number of RRs and then skip past the header */ if (alen < HFIXEDSZ) { return false; } ancount = DNS_HEADER_ANCOUNT(abuf); if (ancount == 0) { return false; } aptr = abuf + NS_HFIXEDSZ;
Why do you use both HFIXEDSZ and NS_HFIXEDSZ? What is the difference?
/* We only care about len from the question data, * so that we can move past hostname */ ret = ares_expand_name(aptr, abuf, alen, &name, &len); ares_free_string(name); if (ret != ARES_SUCCESS) { return false; } /* Skip past the question */ if (aptr + len + NS_QFIXEDSZ > abuf + alen) { ares_free_string(name);
double free of name
return false; } aptr += len + NS_QFIXEDSZ;
It is not necessary to do the addition twice:
aptr += len + NS_QFIXEDSZ; if (aptr > abuf + alen) { return false; }
/* Examine each RR in turn and read the lowest TTL */ for (i = 0; i < ancount; i++) { /* Decode the RR up to the data field. */ ret = ares_expand_name(aptr, abuf, alen, &name, &len); ares_free_string(name); if (ret != ARES_SUCCESS) { return false; } aptr += len; if (aptr + NS_RRFIXEDSZ > abuf + alen) { return false; } rr_len = DNS_RR_LEN(aptr); rr_ttl = DNS_RR_TTL(aptr); if (aptr + rr_len > abuf + alen) { return false; } aptr += NS_RRFIXEDSZ + rr_len; if (ttl > 0) { ttl = MIN(ttl, rr_ttl); } else { ttl = rr_ttl; /* special-case for first TTL */ } } *_ttl = ttl; return true;
}
It looks good otherwise. I got the following compilation error though:
CC src/tests/cmocka/ifp_tests-common_mock_resp.o In file included from /home/pbrezina/workspace/sssd/src/tests/common.h:31:0, from /home/pbrezina/workspace/sssd/src/tests/cmocka/common_mock.h:44, from /home/pbrezina/workspace/sssd/src/tests/cmocka/test_resolv_fake.c:35: /home/pbrezina/workspace/sssd/src/providers/ldap/sdap.h:507:33: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
On Thu, Jan 15, 2015 at 12:00:31PM +0100, Pavel Březina wrote:
On 01/14/2015 04:21 PM, Jakub Hrozek wrote:
On Mon, Dec 15, 2014 at 03:15:37AM +0100, Jakub Hrozek wrote:
Hi,
The attached patch is a self-contained change related to https://fedorahosted.org/sssd/ticket/1884. I also wrote code that uses the TTL values in the failover code: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=srvttl
But when creating the unit tests I stumbled upon: https://fedorahosted.org/sssd/ticket/2533 and especially: https://fedorahosted.org/sssd/ticket/2532
Until these two are fixed, I don't think we can merge the TTL patch. Feel free to review the changes in my git branch, though, I don't think they'd change.
I changed the function itself to return a separate status as return value and fill a pointer with the TTL.
Still waiting for CI to finish, but the amended patch is attached.
static bool resolv_get_ttl(const unsigned char *abuf, const int alen, uint32_t *_ttl) { const unsigned char *aptr; int ret; char *name = NULL; long len; uint32_t ttl = 0; uint32_t rr_ttl; unsigned int rr_len; unsigned int ancount; unsigned int i;
/* Read the number of RRs and then skip past the header */ if (alen < HFIXEDSZ) { return false; }
ancount = DNS_HEADER_ANCOUNT(abuf); if (ancount == 0) { return false; }
aptr = abuf + NS_HFIXEDSZ;
Why do you use both HFIXEDSZ and NS_HFIXEDSZ? What is the difference?
The version without prefix is obsolete, we should use NS_*. It was a copy-n-paste error from c-ares parsing function.
/* We only care about len from the question data, * so that we can move past hostname */ ret = ares_expand_name(aptr, abuf, alen, &name, &len); ares_free_string(name); if (ret != ARES_SUCCESS) { return false; }
/* Skip past the question */ if (aptr + len + NS_QFIXEDSZ > abuf + alen) { ares_free_string(name);
double free of name
Good catch!
return false;
} aptr += len + NS_QFIXEDSZ;
It is not necessary to do the addition twice:
aptr += len + NS_QFIXEDSZ; if (aptr > abuf + alen) { return false;
I guess, as long as we don't access the pointer..
}
/* Examine each RR in turn and read the lowest TTL */ for (i = 0; i < ancount; i++) { /* Decode the RR up to the data field. */ ret = ares_expand_name(aptr, abuf, alen, &name, &len); ares_free_string(name); if (ret != ARES_SUCCESS) { return false; }
aptr += len; if (aptr + NS_RRFIXEDSZ > abuf + alen) { return false; } rr_len = DNS_RR_LEN(aptr); rr_ttl = DNS_RR_TTL(aptr); if (aptr + rr_len > abuf + alen) { return false; } aptr += NS_RRFIXEDSZ + rr_len; if (ttl > 0) { ttl = MIN(ttl, rr_ttl); } else { ttl = rr_ttl; /* special-case for first TTL */ }
}
*_ttl = ttl; return true; }
It looks good otherwise. I got the following compilation error though:
CC src/tests/cmocka/ifp_tests-common_mock_resp.o In file included from /home/pbrezina/workspace/sssd/src/tests/common.h:31:0, from /home/pbrezina/workspace/sssd/src/tests/cmocka/common_mock.h:44, from /home/pbrezina/workspace/sssd/src/tests/cmocka/test_resolv_fake.c:35: /home/pbrezina/workspace/sssd/src/providers/ldap/sdap.h:507:33: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
This one was fun to track down. So because in the tests we use both the system resolver and indirectly the sdap.h include file is present, the function declaration resolved to:
errno_t sdap_parse_deref(TALLOC_CTX *mem_ctx, struct sdap_attr_map_info *minfo, size_t num_maps, LDAPDerefRes *dref, struct sdap_deref_attrs ***(*__res_state()));
I think the simplest solution is to rename the _res parameter into something else..
On 01/21/2015 05:30 PM, Jakub Hrozek wrote:
It looks good otherwise. I got the following compilation error though:
CC src/tests/cmocka/ifp_tests-common_mock_resp.o In file included from /home/pbrezina/workspace/sssd/src/tests/common.h:31:0, from /home/pbrezina/workspace/sssd/src/tests/cmocka/common_mock.h:44, from /home/pbrezina/workspace/sssd/src/tests/cmocka/test_resolv_fake.c:35: /home/pbrezina/workspace/sssd/src/providers/ldap/sdap.h:507:33: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
This one was fun to track down. So because in the tests we use both the system resolver and indirectly the sdap.h include file is present, the function declaration resolved to:
errno_t sdap_parse_deref(TALLOC_CTX *mem_ctx, struct sdap_attr_map_info *minfo, size_t num_maps, LDAPDerefRes *dref, struct sdap_deref_attrs ***(*__res_state()));
I think the simplest solution is to rename the _res parameter into something else..
Nice... this is just sad.
ACK to all patches. I'm sending a rebased version in the attachement, it was just a simple Makefile conflict.
On Tue, Jan 27, 2015 at 11:24:48AM +0100, Pavel Březina wrote:
On 01/21/2015 05:30 PM, Jakub Hrozek wrote:
It looks good otherwise. I got the following compilation error though:
CC src/tests/cmocka/ifp_tests-common_mock_resp.o In file included from /home/pbrezina/workspace/sssd/src/tests/common.h:31:0, from /home/pbrezina/workspace/sssd/src/tests/cmocka/common_mock.h:44, from /home/pbrezina/workspace/sssd/src/tests/cmocka/test_resolv_fake.c:35: /home/pbrezina/workspace/sssd/src/providers/ldap/sdap.h:507:33: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
This one was fun to track down. So because in the tests we use both the system resolver and indirectly the sdap.h include file is present, the function declaration resolved to:
errno_t sdap_parse_deref(TALLOC_CTX *mem_ctx, struct sdap_attr_map_info *minfo, size_t num_maps, LDAPDerefRes *dref, struct sdap_deref_attrs ***(*__res_state()));
I think the simplest solution is to rename the _res parameter into something else..
Nice... this is just sad.
ACK to all patches. I'm sending a rebased version in the attachement, it was just a simple Makefile conflict.
Thank you very much for the rebase. Pushed to master: * bf54fbed126ec3d459af40ea370ffadacd31c76d * 4d7fe714fe74ad242497b2bdbeb7b4e0bf40141f and to sssd-1-12: * 3149069126599133a8fe0c66734df6deb3907dfb * 07d69e93a2d2ba68c2fe67d8fb5de18cf69ba797
sssd-devel@lists.fedorahosted.org