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

[Simo Sorce]

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.

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.

> [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 :-)

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

-- 
Simo Sorce * Red Hat, Inc * New York