diff mbox

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

Message ID alpine.DEB.2.10.1604221400580.60537@buzzword-bingo.mit.edu
State New, archived
Headers

Commit Message

Anders Kaseorg April 22, 2016, 6:25 p.m. UTC 
  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).

Comments

Florian Weimer April 23, 2016, 12:30 p.m. UTC | #1 
* 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.
Anders Kaseorg April 23, 2016, 6:37 p.m. UTC | #2 
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
Anders Kaseorg April 27, 2016, 3:36 p.m. UTC | #3 
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
Florian Weimer April 27, 2016, 6:19 p.m. UTC | #4 
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
Anders Kaseorg April 30, 2016, 11:17 p.m. UTC | #5 
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
Florian Weimer May 2, 2016, 10:02 a.m. UTC | #6 
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
Anders Kaseorg May 2, 2016, 10:16 a.m. UTC | #7 
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
diff mbox

Patch

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);
 }