[SSSD] [PATCH] RESOLV: Add an internal function to read TTL from a DNS packet

Pavel Březina pbrezina at redhat.com
Thu Jan 15 11:00:31 UTC 2015


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]





More information about the sssd-devel mailing list