Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in ub_ctx (event_base will never be freed) #99

Open
WolverinDEV opened this issue Oct 23, 2019 · 9 comments
Open

Memory leak in ub_ctx (event_base will never be freed) #99

WolverinDEV opened this issue Oct 23, 2019 · 9 comments

Comments

@WolverinDEV
Copy link

@WolverinDEV WolverinDEV commented Oct 23, 2019

Hey,
the API method ub_ctx_create_event combined with ub_ctx_delete leaks some memory (24 bytes).
This is because a new event_base will be allocated via ub_libevent_event_base, but never deallocated. Even if we call ub_ctx_delete.

@wcawijngaards

This comment has been minimized.

Copy link
Member

@wcawijngaards wcawijngaards commented Oct 24, 2019

Thanks for the heads up! Added a check that when the event base is set via this method it frees the struct that is alloced.

jedisct1 added a commit to jedisct1/unbound that referenced this issue Oct 29, 2019
* nlnet/master:
  - Fix NLnetLabs#99: Memory leak in ub_ctx (event_base will never be freed).
  Add new configure option `--enable-fully-static` to enable full static build if requested; in relation to NLnetLabs#91.
  Changelog note for NLnetLabs#97. - Merge NLnetLabs#97: manpage: Add missing word on unbound.conf,   from Erethon.
  manpage: Add missing word on unbound.conf
  - drop-tld.diff: adds option drop-tld: yesno that drops 2 label   queries, to stop random floods.  Apply with   patch -p1 < contrib/drop-tld.diff and compile.   From Saksham Manchanda (Secure64).  Please note that we think this   will drop DNSKEY and DS lookups for tlds and hence break DNSSEC   lookups for downstream clients.
@fobser

This comment has been minimized.

Copy link
Contributor

@fobser fobser commented Dec 18, 2019

This is only true for ub_libevent_event_base in util/ub_event_pluggable.c.
The one in util/ub_event.c does not allocate memory but uses the passed in event_base.
So now I'm facing a use-after-free and most of the time event_dispatch() just returns.

bob-beck pushed a commit to openbsd/src that referenced this issue Dec 18, 2019
NLnetLabs/unbound#99
ub_ctx_delete would free the passed in event_base leading to
use-after-free since libunbound never allocated the memory and
unwind expects to continue using the event_base.
@WolverinDEV

This comment has been minimized.

Copy link
Author

@WolverinDEV WolverinDEV commented Dec 19, 2019

Well this is really an major issue. It has been implemented falsely.
Lookup this part of code:

ctx->event_base = ub_libevent_event_base(eb);
if (!ctx->event_base) {
ub_ctx_delete(ctx);
return NULL;
}
ctx->event_base_malloced = 1;

I'll create a PR in a few min which will fix this flaw

@ralphdolmans ralphdolmans reopened this Dec 19, 2019
WolverinDEV added a commit to WolverinDEV/unbound that referenced this issue Dec 19, 2019
@ralphdolmans

This comment has been minimized.

Copy link
Member

@ralphdolmans ralphdolmans commented Dec 20, 2019

Hi Florian,

As far as I can tell libunbound always uses ub_event_pluggable.c, in which case this should not be an issue. That is assuming you build Unbound with the shipped Makefile. I see in the OpenBSD code that you are using your own Makefile which uses the ub_event.c, is that intentional?

@fobser

This comment has been minimized.

Copy link
Contributor

@fobser fobser commented Dec 20, 2019

Edit: nevermind
I'm not sure, however, this is the knob in configure if I'm not mistaken:
--enable-event-api Enable (experimental) pluggable event base
libunbound API installed to unbound-event.h
It says experimental...

@fobser

This comment has been minimized.

Copy link
Contributor

@fobser fobser commented Dec 20, 2019

unwind(8), which is the only consumer of libunbound in OpenBSD base, uses libevent so it felt natural to also stick to that.
I could investigate to switch to the plugable version if that's better / supported.

@fobser

This comment has been minimized.

Copy link
Contributor

@fobser fobser commented Dec 20, 2019

I switched unwind over to ub_event_pluggable,c and it seems to be working.
Needs a whee bit more testing though.
In any case, I don't have a strong opinion in which direction this should go.
I'd suggest to delete ub_event.c though if that's deprecated / should not be used.

@ralphdolmans

This comment has been minimized.

Copy link
Member

@ralphdolmans ralphdolmans commented Dec 23, 2019

I'm not sure, however, this is the knob in configure if I'm not mistaken:
--enable-event-api Enable (experimental) pluggable event base

I understand the confusion. I think we should remove the experimental part, especially since it is there already for 6 years. Note that the flag does not enable or disable the pluggable event base, it is about installing the header file. The ub_event_pluggable.c is always used for libunbound.

I'd suggest to delete ub_event.c though if that's deprecated / should not be used.

It is actually used when using Unbound in daemon mode, but not when using libunbound.

I switched unwind over to ub_event_pluggable,c and it seems to be working.

Yes, I think this is the right approach.

bob-beck pushed a commit to openbsd/src that referenced this issue Dec 23, 2019
ub_event_pluggable.c instead of ub_event.c.
( NLnetLabs/unbound#99 )
We have been the odd one out, so switch to ub_event_pluggable, too.
@fobser

This comment has been minimized.

Copy link
Contributor

@fobser fobser commented Dec 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.