On Thu, Jun 08, 2006 at 02:29:23PM -0700, William Ahern wrote:
> I'm not sure if "re-entrancy" is the right term, but there is a critical
> issue in Ares. Maybe it's been discussed before.
>
> If a user calls ares_destroy() from a callback, then the library will access
> invalid memory as it unwinds. One instance is in end_query in
> ares_process.c.
>
> My typical solution is to push a pointer to a flag in automatic storage of
> the call frame onto a stack of the main context object before every
> callback. The object is popped when the callback returns (of course checking
> the flag first).
>
Phew. That was a long night. Attached is a patch to implement a per-channel
call frame stack. I meticulously traced the call graph by hand and I believe
I have placed guards and/or rearranged code at all the important points to
remove the potential for double frees and invalid accesses.
I believe this really was a re-entrancy problem, since the same issues could
have cropped up if ares_process() or ares_query() was called from a
callback. Even this should work now, too, since I reordered critical
operations.
In my tests everything just works :) I used valgrind on Linux, and OpenBSD
(which interestingly caught this problem whereas valgrind didn't; or maybe I
wasn't paying attention or used the wrong switches).
This patch is against the 20060608 snapshot. Also, this patch includes my
EAGAIN checks which again were forgotten (or maybe I missed some critiques).
Without these EAGAIN checks queries will fail under heavy load. It's not
pretty to see a handful of threaded getnameinfo() calls destroy c-ares
in performance and query success rate ;)