fix(libsinsp): memleak in sinsp_dns_manager

[incertum]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
#0 0x7f84317d54c8 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:95
#1 0x56319abb44cc in sinsp_dns_manager::match(char const*, int, void*, unsigned long) /build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libsinsp/dns_manager.cpp:121

add destructor to class sinsp_dns_manager to ensure m_resolver is always released when the object goes out of scope

@LucaGuerra

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[incertum]

/milestone 0.14.0

[FedeDP]

note: cleanup is idempotent:

void sinsp_dns_manager::cleanup()
{
	if(m_resolver)
	{
		m_exit_signal.set_value();
		m_resolver->join();
		m_resolver = NULL;
		m_exit_signal = std::promise<void>();
	}
}

LGTM :)

[FedeDP]

/approve

[poiana]

LGTM label has been added.

Git tree hash: 984e60622905122157088003245a4b0da520ca9d

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [FedeDP,LucaGuerra,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[incertum]

@LucaGuerra @FedeDP and @federico-sysdig while it appeared to make sense adding the destructor I am still seeing the memleak. Seems like we need to invest more and actually look into this more thoroughly and add a test so we check it with asan in the CI.

[federico-sysdig]

@LucaGuerra @FedeDP and @federico-sysdig while it appeared to make sense adding the destructor I am still seeing the memleak. Seems like we need to invest more and actually look into this more thoroughly and add a test so we check it with asan in the CI.

@incertum I'm not sure if I can be of great help here... The only thing I find odd is keeping the thread as a pointer and, on top of that, never deleting it. I'd change the thread data member as a normal std::thread object or, at the very least, use a unique_ptr.

[incertum]

@LucaGuerra @FedeDP and @federico-sysdig while it appeared to make sense adding the destructor I am still seeing the memleak. Seems like we need to invest more and actually look into this more thoroughly and add a test so we check it with asan in the CI.

@incertum I'm not sure if I can be of great help here... The only thing I find odd is keeping the thread as a pointer and, on top of that, never deleting it. I'd change the thread data member as a normal std::thread object or, at the very least, use a unique_ptr.

SGTM and like a great improvement regardless. Do you want to author the PR? Thanks in advance 🙏 !

[federico-sysdig]

@incertum Feel free to proceed as you wish. It's a small thing. Thanks for asking.