[PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Anders Kaseorg]

2016-04-22  Anders Kaseorg  <andersk@mit.edu>

	[BZ #19573]
	* hesiod/hesiod.c (hesiod_end): Only call res_nclose(ctx->res) if
	ctx->free_res is nonnull, to prevent a crash on res_nclose(&res)
	introduced by commit 2212c1420c92a33b0e0bd9a34938c9814a56c0f7
	(Simplify handling of nameserver configuration in resolver).

diff --git a/hesiod/hesiod.c b/hesiod/hesiod.c
index 657dabe..a540382 100644
--- a/hesiod/hesiod.c
+++ b/hesiod/hesiod.c
@@ -152,12 +152,12 @@ hesiod_end(void *context) {
 	struct hesiod_p *ctx = (struct hesiod_p *) context;
 	int save_errno = errno;
 
-	if (ctx->res)
+	if (ctx->res && ctx->free_res) {
 		res_nclose(ctx->res);
+		(*ctx->free_res)(ctx->res);
+	}
 	free(ctx->RHS);
 	free(ctx->LHS);
-	if (ctx->res && ctx->free_res)
-		(*ctx->free_res)(ctx->res);
 	free(ctx);
 	__set_errno(save_errno);
 }

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Florian Weimer]

* Anders Kaseorg:

> 2016-04-22  Anders Kaseorg  <andersk@mit.edu>
>
> 	[BZ #19573]
> 	* hesiod/hesiod.c (hesiod_end): Only call res_nclose(ctx->res) if
> 	ctx->free_res is nonnull, to prevent a crash on res_nclose(&res)
> 	introduced by commit 2212c1420c92a33b0e0bd9a34938c9814a56c0f7
> 	(Simplify handling of nameserver configuration in resolver).
>
> diff --git a/hesiod/hesiod.c b/hesiod/hesiod.c
> index 657dabe..a540382 100644
> --- a/hesiod/hesiod.c
> +++ b/hesiod/hesiod.c
> @@ -152,12 +152,12 @@ hesiod_end(void *context) {
>  	struct hesiod_p *ctx = (struct hesiod_p *) context;
>  	int save_errno = errno;
>  
> -	if (ctx->res)
> +	if (ctx->res && ctx->free_res) {
>  		res_nclose(ctx->res);
> +		(*ctx->free_res)(ctx->res);
> +	}

Please use GNU style (braces on separate lines, two-space
indentation).

>  	free(ctx->RHS);
>  	free(ctx->LHS);
> -	if (ctx->res && ctx->free_res)
> -		(*ctx->free_res)(ctx->res);

This is one way to fix this bug.  Its correctness depends on whether
we export in any way the hesiod functionality.  I thought we did, but
libhesiod is actually a separate thing, and checking with eu-readelf,
I don't see any exports of the helper functions.

If there are no external users of the callback mechanism, we do not
have to worry about the behavioral change.

I would welcome a quick double-check on this aspect.

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Anders Kaseorg]

Thanks for the review!

On Sat, 23 Apr 2016, Florian Weimer wrote:
> Please use GNU style (braces on separate lines, two-space
> indentation).

hesiod/hesiod.c is not in GNU style; I preserved its existing style as per 
https://sourceware.org/glibc/wiki/Style_and_Conventions#Files_not_formatted_according_to_the_GNU_standard.

> This is one way to fix this bug.  Its correctness depends on whether we 
> export in any way the hesiod functionality.  I thought we did, but 
> libhesiod is actually a separate thing, and checking with eu-readelf, I 
> don't see any exports of the helper functions.
> 
> If there are no external users of the callback mechanism, we do not have 
> to worry about the behavioral change.
> 
> I would welcome a quick double-check on this aspect.

I agree: this file is only used in nss_hesiod, whose only exported symbols 
are the NSS interface.

Also, the test I used is the same as what __hesiod_res_set already uses 
before calling res_nclose.  This may not be strong evidence by itself, 
because it happens that __hesiod_res_set is never called in a way that 
satisfies this test, but at least it’s consistent.

Anders

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Anders Kaseorg]

On Sat, 23 Apr 2016, Anders Kaseorg wrote:
> Thanks for the review!
> 
> > […]
> 
> hesiod/hesiod.c is not in GNU style; I preserved its existing style as per 
> https://sourceware.org/glibc/wiki/Style_and_Conventions#Files_not_formatted_according_to_the_GNU_standard.
> 
> > […]
> 
> I agree: this file is only used in nss_hesiod, whose only exported symbols 
> are the NSS interface.
> 
> Also, the test I used is the same as what __hesiod_res_set already uses 
> before calling res_nclose.  This may not be strong evidence by itself, 
> because it happens that __hesiod_res_set is never called in a way that 
> satisfies this test, but at least it’s consistent.

Is this ready to be committed, or do you still have concerns?

Anders

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Florian Weimer]

On 04/27/2016 05:36 PM, Anders Kaseorg wrote:
> On Sat, 23 Apr 2016, Anders Kaseorg wrote:
>> Thanks for the review!
>>
>>> […]
>>
>> hesiod/hesiod.c is not in GNU style; I preserved its existing style as per
>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Files_not_formatted_according_to_the_GNU_standard.
>>
>>> […]
>>
>> I agree: this file is only used in nss_hesiod, whose only exported symbols
>> are the NSS interface.
>>
>> Also, the test I used is the same as what __hesiod_res_set already uses
>> before calling res_nclose.  This may not be strong evidence by itself,
>> because it happens that __hesiod_res_set is never called in a way that
>> satisfies this test, but at least it’s consistent.
>
> Is this ready to be committed, or do you still have concerns?

Yes please, this change looks fine.

I'll add Hesiod tests to the resolver test suite eventually.

Florian

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Anders Kaseorg]

On Wed, 27 Apr 2016, Florian Weimer wrote:
> On 04/27/2016 05:36 PM, Anders Kaseorg wrote:
> > Is this ready to be committed, or do you still have concerns?
> 
> Yes please, this change looks fine.

Do I need to do anything else?  In case it wasn’t clear, I don’t have 
commit access.

Anders

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Florian Weimer]

On 05/01/2016 01:17 AM, Anders Kaseorg wrote:
> On Wed, 27 Apr 2016, Florian Weimer wrote:
>> On 04/27/2016 05:36 PM, Anders Kaseorg wrote:
>>> Is this ready to be committed, or do you still have concerns?
>>
>> Yes please, this change looks fine.
>
> Do I need to do anything else?  In case it wasn’t clear, I don’t have
> commit access.

I wasn't aware of that, sorry.

I have looked at the patch again, and I think it is technically 
incorrect.  The proper way to fix this would be to install a resolver 
callback which does nothing, and free the resolver state only if there 
is *no* callback (because we know we own it).

But we only ever use the thread-local resolver state anyway, so I'm just 
going to remove this code.

Florian

Re: [PATCH] hesiod_end: Do not call res_nclose(&_res) [BZ #19573]

[Anders Kaseorg]

On Mon, 2 May 2016, Florian Weimer wrote:
> I wasn't aware of that, sorry.
> 
> I have looked at the patch again, and I think it is technically 
> incorrect. The proper way to fix this would be to install a resolver 
> callback which does nothing, and free the resolver state only if there 
> is *no* callback (because we know we own it).
> 
> But we only ever use the thread-local resolver state anyway, so I'm just 
> going to remove this code.

I will be happy to see the problem resolved however you see fit.

(If you think this code is incorrect, consider also fixing/deleting the 
code in __hesiod_res_set that it was copied from.)

Thanks,
Anders