[SSSD] [PATCH] c-ares 1.7.0 related changes

Martin Nagy mnagy at redhat.com
Tue Dec 8 13:20:46 UTC 2009


On Mon, 2009-12-07 at 20:03 +0100, Jakub Hrozek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 12/02/2009 03:07 PM, Martin Nagy wrote:
> > 
> > I think we should just push these patches (after the review is done) and
> > then disable them in the build & configure system like Stephen did for
> > ELAPI (although FWIW, I think it would be nice to keep the tests
> > running).
> > 
> 
> OK, I just think this should be a separate patch - hence 0003.
> 
> >> [PATCH 1/2] Change ares usage to be c-ares 1.7.0 compatible
> >> * Rename structure accordingly to ares upstream
> >> * Use new ares parsing functions in the wrappers
> >> * fix tests for ares 1.7
> > 
> >>From rewrite_talloc_srv_reply():
> > +        if (ptr->host == NULL) {
> > +            talloc_free(ptr);
> >              return ENOMEM;
> >          }
> > +
> > I believe this should be talloc_free(new_list), otherwise you might try
> > to free a pointer pointing into middle of a talloc'ed memory. Also, is
> 
> Thanks, fixed.
> 
> > it wise to allocate an array for a linked list? I would recommend
> > allocating each list item separately using mem_ctx and maybe provide a
> > free function that would free all the items.
> 
> OK, each item is now allocated using talloc_zero in the loop. I just
> think the other items including the SRV/TXT payload should be allocated
> using new_list as a context, that way you can just talloc_free(new_list).
> 
> > 
> > +        ptr = ptr->next;
> > +        old_list = old_list->next;
> > This is also bad, since ptr->next is not initialized.
> > 
> 
> For whatever reason I thought talloc_array was using talloc_zero under
> the hood. Using talloc_zero for every item separately now takes care of it.

Unfortunately, that still doesn't solve the issue. These functions are
surprisingly difficult to get right. The way the code is now, you don't
link the list items. I also just noticed that we don't handle cases were
*reply_list == NULL. Also, if we return in the middle of the function,
we forget to free the original reply_list. Also,
ares_free_data(reply_list); is wrong, because you're passing ares the
double pointer, the correct way would be to pass *reply_list (and even
better, simply old_list).

Seeing as these functions are so much tricky, I propose one of the
following to be done:

1. Fix the issues outlined, create an extensive test similar to
test_copy_hostent in resolv-test.c (for at least 2 items in the list).
Include a special case in the test calling the function with NULL list,
etc. Also don't forget about leak checks. Try running the test with
mudflap to make sure we don't access any pointers that we shouldn't.
This way, I'd be pretty confident that the code is good.

or

2. Simply remove the functions and use a memory holder instead, see
server/util/memory.c, sss_mem_attach().

These points apply to both functions of course.

> > Same goes for rewrite_talloc_txt_reply().
> > 
> 
> Also fixed.
> 
> >> [PATCH 2/2] Import ares 1.7.0 helpers
> >> Synchronizes the bundled helpers with the changes done during the c-ares
> >> 1.7.0 development in order for the SRV and TXT parsing routines to work
> >> also with pre-1.7.0 ares versions.
> > 
> > Sorry, I didn't yet have time to review it, I will do it later (tomorrow
> > probably).
> 
> The attached patch 0002 fixes the issue you outlined in the other reply
> to the thread.
> 
> 	Jakub

0002 and 0003 are looking good.

Martin




More information about the sssd-devel mailing list