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

[Jakub Hrozek]

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.

[Jakub Hrozek]

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.

[Jakub Hrozek]

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/ee3ec0f68db9be1bb11ba0d20b852c8...

[Pavel Březina]

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]

[Jakub Hrozek]

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..

[Pavel Březina]

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.

[Jakub Hrozek]

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