[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