Request for comment for async resolution of DNS names

Jeremy Allison jra at samba.org
Thu Feb 9 19:30:29 UTC 2017


On Thu, Feb 09, 2017 at 11:18:37AM -0800, Matthieu Patou wrote:
> Volker,
> On 02/09/2017 12:33 AM, Volker Lendecke wrote:
> > On Wed, Feb 08, 2017 at 11:10:11AM -0800, Matthieu Patou wrote:
> >> +static void resolve_one_name(void *private_data)
> >> +{
> >> +	struct dns_rr_srv *rr = private_data;
> >> +	struct addrinfo *res = NULL;
> >> +	struct addrinfo *p;
> >> +	int num_ips = 0;
> >> +	if (!interpret_string_addr_internal(&res, rr->hostname, 0)) {
> >> +		return;
> >> +	}
> >> +	/* Add in every IP from the lookup. How many is that ? */
> >> +	for (p = res; p; p = p->ai_next) {
> >> +		if (num_ips != 0) {
> >> +			if (NULL == (rr->ss_s = talloc_realloc(rr->ss_s, rr->ss_s,
> >> +					struct sockaddr_storage, num_ips + 1)))
> >> +			{
> >> +				return;
> >> +			}
> >> +		}
> >> +		memcpy(&rr->ss_s[num_ips], p->ai_addr, p->ai_addrlen);
> >> +		if (is_zero_addr(&rr->ss_s[num_ips])) {
> >> +			continue;
> >> +		}
> >> +		num_ips++;
> >> +	}
> >> +	rr->num_ips = num_ips;
> >> +	if (res) {
> >> +		freeaddrinfo(res);
> >> +	}
> >> +}
> > For my taste, this is much too complex for a thread job function. It
> > calls minterpret_string_addr_internal, talloc_realloc and is_zero_addr
> > where I'd have to think very long about whether they are safe to be
> > called in a thread. With this change, interpret_string_addr_internal
> > gets a completely fresh requirement that we would have to document
> > very clearly: Thread safety. I would not hold my breath for us being
> > good enough in our review process to always be alert that potentially
> > thread-unsafe changes will be blocked for interpret_string_addr_internal.
> >
> > That's what I meant with my comment to do only the absolute bare minimum
> > in a threaded function to make sure it's thread-safe.
> >
> > Can you restructure the code to only call getaddrinfo and nothing else
> > in the threaded job.
> Thanks for the feedback, I wasn't sure that if I called getaddrinfo
> directly someone wouldn't argue that interpret_string_addr_internal was
> meant for that and that I should use it.

interpret_string_addr_internal() calls strchr_m() which calls
get_iconv_handle(). I am 100% sure get_iconv_handle() is not
thread-safe.



More information about the samba-technical mailing list