Message ID | alpine.DEB.2.10.1604221400580.60537@buzzword-bingo.mit.edu |
---|---|
State | New |
Headers | show |
* 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.
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
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
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
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
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
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 --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); }