[Freeipa-devel] [PATCHES] c-ares integration

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

All three patches with corrections are attached. Some comments inline.

On 07/21/2009 08:39 PM, Simo Sorce wrote:
> On Tue, 2009-07-21 at 19:09 +0200, Jakub Hrozek wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hello,
>> The three attached patches integrate the c-ares async resolver library
>> into SSSD. It is a follow-up to the code posted on this list earlier [1].
>>
>> [PATCH 1/3] Async DNS integration
>> This patch is the actual integration work with interface integrated with
>> tevent and talloc which follows the _send/_recv notation. It also
>> contains code to support build (like configure checks).
> 
> I am a bit concerned about memory hierarchies here.
> 
> In fd_event_add() you add a new event context allocating it on the
> tevent_context memory context:
> +    fde = tevent_add_fd(ctx->ev_ctx, ctx, s, TEVENT_FD_READ, fd_input_available, watch);
> 
> This is bad because if I talloc_free() the resolv context later on on,
> all the events will not be freed but will stay around and watch->ctx
> will point to freed memory. Allocating the watch itself on the
> tevent_context is equally bad.
> 

We changed the memory contexts a bit -- the watch structure is now
allocated on resolv_ctx and the watch instance is in turn passed to
tevent_add_fd to allocate the tevent_fd on.

We also added a destructor to the watch instances, which invalidates the
file descriptor before freeing the memory.

> Another potential problem I see is that, if I read the code correctly,
> resolv_gethostbyname_done() maybe called even if the original request
> was freed, this may cause the code to segfault when it calls
> tevent_req_XXX(req, ...) functions and tries to access freed memory.
> 

If resolv_ctx is freed, destructor is called, which calls ares_destroy -
and ares would cancel all the pending requests from the queue, so
resolv_gethostbyname_done (which is a callback called from
ares) would not be called.

>> [PATCH 2/3] Add ares helpers into sssd
>> The second patch contains ares_parse_srv_reply() and
>> ares_parse_txt_reply() routines that I sent to ares upstream. Also
>> contains configure-time detection of these in the system ares library
>> and builds them only if necessary.
> 
> I am not an expert in c-ares, but it looks like the code is sane, one
> minor thing though. Unless upstream demands copyright assignment to MIT,
> I think we can maintain the copyright, and even if assignment is
> required we can do it only with the patches submitted to ares, no need
> to do so with code into the sssd repo itself.
> In any case the (C) is certainly not 1998. That is, unless you traveled
> back in time to write it :-)
> 

OK, I simply copied what I sent to upstream. They use (C) 1998 even for
new code (which struck me, too, but as these parsing functions were my
first submission, I did not want to propose too many changes at once..).

As discussed on IRC, since the code is based on other parsing function
in c-ares, I also left the original MIT notice there.

>> [PATCH 3/3] Add async resolver tests
>> A basic unit test. One of the tests resolves a name on the Internet, so
>> it must be explicitly enabled.
> 
> This look fine.
> 
> Maybe when you take in account my comment to 1/3 you may want to add a
> test that does something like this:
> 
> make a _send call then set a timer of 1 second and free the resolv_ctx,
> add another time of 1 second and then terminate. Perform this test with
> a firewall or a network cable disconnected so that you run into delays.
> Do the same but instead of freeing the resolv_ctx, simply free the
> tevent_req pointer returned by the _send() function.
> 
> If the code survives without segfaults then it is probably correct.
> Ping me on IRC if you need some clarification on how to attain that.
> 
> Simo.
> 

I added these two testcases to the test code. Survived in both networked
and offline environment.

Thank you for the review,
Jakub

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkpnVdEACgkQHsardTLnvCV3OQCfUMmm7FT49/f5ZBo7s62lbq2Q
4K8AnAvjvSHwGL1hN574KMijEb5AuPu+
=7KuA
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Async-DNS-integration.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090722/9cd097c8/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Add-ares-helpers-into-sssd.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090722/9cd097c8/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-Add-async-resolver-tests.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090722/9cd097c8/attachment-0002.ksh>