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

refactor(libsinsp): use smart pointer for m_resolver in sinsp_dns_manager #1558

Merged
merged 4 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions userspace/libsinsp/dns_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ bool sinsp_dns_manager::match(const char *name, int af, void *addr, uint64_t ts)
#if defined(HAS_CAPTURE) && !defined(CYGWING_AGENT) && !defined(_WIN32) && !defined(__EMSCRIPTEN__)
if(!m_resolver)
{
m_resolver = new std::thread(sinsp_dns_resolver::refresh, m_erase_timeout, m_base_refresh_timeout, m_max_refresh_timeout, m_exit_signal.get_future());
m_resolver = std::make_unique<std::thread>(sinsp_dns_resolver::refresh, m_erase_timeout, m_base_refresh_timeout, m_max_refresh_timeout, m_exit_signal.get_future());
}

std::string sname = std::string(name);
Expand Down Expand Up @@ -195,7 +195,7 @@ void sinsp_dns_manager::cleanup()
{
m_exit_signal.set_value();
m_resolver->join();
m_resolver = NULL;
m_resolver.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. Good catch!

m_exit_signal = std::promise<void>();
}
}
2 changes: 1 addition & 1 deletion userspace/libsinsp/dns_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class sinsp_dns_manager
// used to let m_resolver know when to terminate
std::promise<void> m_exit_signal;

std::thread *m_resolver;
std::unique_ptr<std::thread> m_resolver;

uint64_t m_erase_timeout;
uint64_t m_base_refresh_timeout;
Expand Down
2 changes: 2 additions & 0 deletions userspace/libsinsp/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ set(LIBSINSP_UNIT_TESTS_SOURCES
container_info.ut.cpp
sinsp_utils.ut.cpp
state.ut.cpp
dns_manager.ut.cpp
eventformatter.ut.cpp
savefile.ut.cpp
sinsp_stats.ut.cpp
Expand Down Expand Up @@ -178,6 +179,7 @@ endif()
target_link_libraries(unit-test-libsinsp
"${GTEST_LIB}"
"${GTEST_MAIN_LIB}"
"${TBB_LIB}"
sinsp
)

Expand Down
33 changes: 33 additions & 0 deletions userspace/libsinsp/test/dns_manager.ut.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// SPDX-License-Identifier: Apache-2.0
/*
Copyright (C) 2023 The Falco Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

*/

#if defined(HAS_CAPTURE) && !defined(CYGWING_AGENT) && !defined(_WIN32) && !defined(__EMSCRIPTEN__)
#include <dns_manager.h>
#include <gtest/gtest.h>

TEST(sinsp_dns_manager, simple_dns_manager_invocation)
{
// Simple dummy test to assert that sinsp_dns_manager is invocated correctly
// and not leaking memory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we run the test suite under valgrind in CI? Do we actually detect the (lack of) memory leak?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run ASan in all libs tests, which will detect memory leaks.

const char* name = "bogus";
uint64_t ts = 11111111111111;
uint32_t addr = 111111;
bool result = sinsp_dns_manager::get().match(name, AF_INET, &addr, ts);
ASSERT_FALSE(result);
}
#endif