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]