Skip to content

Commit aa1b484

Browse files
haoxian2pcolberg
authored andcommitted
Fixed coverity in acl_hal_mmd.cpp: Resource leak (RESOURCE_LEAK)
mmd_libraries were not closed after execution, which causes memory leaks. But mmd_libraries cannot be closed trivially since closing the library invalidates the symbol return by dlsym(), which means calling the function further down is undefined behaviour. To solve this issues, I added a `dl_wrapper_t` vector as a global initialized vector that would be deallocated when closing the runtime library. This vector would contain all the opened libraries that would be closed by the destructor of `dl_wrapper_t` once the vector deallocates.
1 parent 5ea8740 commit aa1b484

File tree

1 file changed

+44
-24
lines changed

1 file changed

+44
-24
lines changed

src/acl_hal_mmd.cpp

Lines changed: 44 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
#include <cstring>
1717
#include <fstream>
1818
#include <iostream>
19+
#include <memory>
20+
#include <optional>
1921
#include <string>
2022
#include <vector>
2123
#ifdef __linux__
@@ -188,8 +190,6 @@ unsigned acl_convert_mmd_capabilities(unsigned mmd_capabilities);
188190
const static size_t MIN_SOF_SIZE = 1;
189191
const static size_t MIN_PLL_CONFIG_SIZE = 1;
190192

191-
std::vector<acl_mmd_dispatch_t> internal_mmd_dispatch;
192-
193193
// Dynamically load board mmd & symbols
194194
static size_t num_board_pkgs;
195195
static double min_MMD_version = DBL_MAX;
@@ -434,14 +434,26 @@ static void *my_dlsym(void *library, const char *function_name,
434434
}
435435

436436
static void my_dlclose(void *library) {
437-
acl_assert_locked();
438437
#ifdef _WIN32
439438
FreeLibrary((HMODULE)library);
440439
#else
441440
dlclose(library);
442441
#endif
443442
}
444443

444+
struct acl_mmd_library {
445+
struct deleter {
446+
void operator()(void *library) { my_dlclose(library); }
447+
};
448+
449+
using pointer = std::unique_ptr<void, deleter>;
450+
451+
pointer ptr;
452+
acl_mmd_dispatch_t dispatch;
453+
};
454+
455+
static std::vector<acl_mmd_library> internal_mmd_dispatch;
456+
445457
cl_bool l_load_board_functions(acl_mmd_dispatch_t *mmd_dispatch,
446458
const char *library_name, void *mmd_library,
447459
char *error_msg) {
@@ -572,7 +584,7 @@ cl_bool l_load_single_board_library(const char *library_name,
572584
return CL_FALSE;
573585
}
574586
#endif
575-
auto *mmd_library = my_dlopen(library_name, &error_msg);
587+
acl_mmd_library::pointer mmd_library{my_dlopen(library_name, &error_msg)};
576588
if (!mmd_library) {
577589
std::cout << "Error: Could not load board library " << library_name;
578590
if (error_msg && error_msg[0] != '\0') {
@@ -583,16 +595,16 @@ cl_bool l_load_single_board_library(const char *library_name,
583595
}
584596

585597
auto *test_symbol =
586-
my_dlsym(mmd_library, "aocl_mmd_get_offline_info", &error_msg);
598+
my_dlsym(mmd_library.get(), "aocl_mmd_get_offline_info", &error_msg);
587599
if (!test_symbol) {
588600
// On Linux, for custom libraries close the library (which was opened
589601
// locally) and then reopen globally. For Windows, there is no option (i.e.
590602
// it is always global)
591603
#ifdef __linux__
592-
my_dlclose(mmd_library);
604+
mmd_library.reset();
593605
ACL_HAL_DEBUG_MSG_VERBOSE(
594606
1, "This library is a custom library. Opening globally.\n");
595-
mmd_library = my_dlopen_global(library_name, &error_msg);
607+
mmd_library.reset(my_dlopen_global(library_name, &error_msg));
596608
if (!mmd_library) {
597609
std::cout << "Error: Could not load custom library " << library_name;
598610
if (error_msg && error_msg[0] != '\0') {
@@ -601,17 +613,20 @@ cl_bool l_load_single_board_library(const char *library_name,
601613
std::cout << "\n";
602614
return CL_FALSE;
603615
}
616+
// FIXME mmd_library goes out of scope at the end of this function. How is
617+
// this custom library used and where should the handle be stored?
604618
#endif
605619
} else {
606620
if (load_libraries) {
607-
auto result =
608-
l_load_board_functions(&(internal_mmd_dispatch[num_boards_found]),
609-
library_name, mmd_library, error_msg);
621+
auto result = l_load_board_functions(
622+
&internal_mmd_dispatch[num_boards_found].dispatch, library_name,
623+
mmd_library.get(), error_msg);
610624
if (result == CL_FALSE) {
611625
std::cout << "Error: Could not load board library " << library_name
612626
<< " due to failure to load symbols\n";
613627
return result;
614628
}
629+
std::swap(mmd_library, internal_mmd_dispatch[num_boards_found].ptr);
615630
}
616631
++num_boards_found;
617632
}
@@ -1134,7 +1149,7 @@ void l_close_device(unsigned int physical_device_id,
11341149
return;
11351150
}
11361151

1137-
static acl_mmd_dispatch_t *get_msim_mmd_layer() {
1152+
static std::optional<acl_mmd_library> get_msim_mmd_layer() {
11381153
#ifdef _WIN32
11391154

11401155
const char *acl_root_dir = acl_getenv("INTELFPGAOCLSDKROOT");
@@ -1159,7 +1174,7 @@ static acl_mmd_dispatch_t *get_msim_mmd_layer() {
11591174
ipa_simulator ? mmd_lib_name_ipa : mmd_lib_name_aoc;
11601175

11611176
char *error_msg = nullptr;
1162-
auto *mmd_lib = my_dlopen(mmd_lib_name, &error_msg);
1177+
acl_mmd_library::pointer mmd_lib{my_dlopen(mmd_lib_name, &error_msg)};
11631178
typedef acl_mmd_dispatch_t *(*fcn_type)();
11641179
if (!mmd_lib) {
11651180
std::cout << "Error: Could not load simulation MMD library "
@@ -1168,17 +1183,17 @@ static acl_mmd_dispatch_t *get_msim_mmd_layer() {
11681183
std::cout << " (error_msg: " << error_msg << ")";
11691184
}
11701185
std::cout << "\n";
1171-
return nullptr;
1186+
return {};
11721187
}
1173-
auto *sym = my_dlsym(mmd_lib, sym_name, &error_msg);
1188+
auto *sym = my_dlsym(mmd_lib.get(), sym_name, &error_msg);
11741189
if (!sym) {
11751190
std::cout << "Error: Symbol " << sym_name
11761191
<< " not found in simulation MMD library ";
11771192
if (error_msg && error_msg[0] != '\0') {
11781193
std::cout << "(message: " << error_msg << ")";
11791194
}
11801195
std::cout << "\n";
1181-
return nullptr;
1196+
return {};
11821197
}
11831198

11841199
// Now call the function. Ignore the Windows cast to fcn pointer
@@ -1187,10 +1202,15 @@ static acl_mmd_dispatch_t *get_msim_mmd_layer() {
11871202
#pragma warning(push)
11881203
#pragma warning(disable : 4055)
11891204
#endif
1190-
return ((fcn_type)sym)();
1205+
acl_mmd_dispatch_t *mmd_dispatch = ((fcn_type)sym)();
11911206
#ifdef _MSC_VER
11921207
#pragma warning(pop)
11931208
#endif
1209+
1210+
auto library = std::make_optional<acl_mmd_library>();
1211+
std::swap(library->ptr, mmd_lib);
1212+
library->dispatch = *mmd_dispatch;
1213+
return library;
11941214
}
11951215

11961216
ACL_HAL_EXPORT const acl_hal_t *
@@ -1255,11 +1275,11 @@ acl_mmd_get_system_definition(acl_system_def_t *sys,
12551275
if (use_offline_only == ACL_CONTEXT_MPSIM) {
12561276

12571277
// Substitute the simulator MMD layer.
1258-
auto *result = get_msim_mmd_layer();
1259-
if (!result)
1278+
auto result = get_msim_mmd_layer();
1279+
if (!result) {
12601280
return nullptr;
1261-
else
1262-
internal_mmd_dispatch.push_back(*result);
1281+
}
1282+
internal_mmd_dispatch.push_back(std::move(*result));
12631283

12641284
ACL_HAL_DEBUG_MSG_VERBOSE(1, "Use simulation MMD\n");
12651285
num_board_pkgs = 1;
@@ -1269,7 +1289,7 @@ acl_mmd_get_system_definition(acl_system_def_t *sys,
12691289
internal_mmd_dispatch.resize(num_board_pkgs);
12701290
ACL_HAL_DEBUG_MSG_VERBOSE(1, "Board MMD is statically linked\n");
12711291

1272-
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[0];
1292+
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[0].dispatch;
12731293

12741294
dispatch.library_name = "runtime_static";
12751295
dispatch.mmd_library = nullptr;
@@ -1345,7 +1365,7 @@ acl_mmd_get_system_definition(acl_system_def_t *sys,
13451365
sys->num_devices = 0;
13461366
num_physical_devices = 0;
13471367
for (unsigned iboard = 0; iboard < num_board_pkgs; ++iboard) {
1348-
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[iboard];
1368+
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[iboard].dispatch;
13491369

13501370
dispatch.aocl_mmd_get_offline_info(AOCL_MMD_VERSION, sizeof(buf), buf,
13511371
NULL);
@@ -1417,7 +1437,7 @@ int acl_hal_mmd_try_devices(cl_uint num_devices, const cl_device_id *devices,
14171437
// names might get cached by various routines
14181438

14191439
for (unsigned iboard = 0; iboard < num_board_pkgs; ++iboard) {
1420-
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[iboard];
1440+
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[iboard].dispatch;
14211441

14221442
dispatch.aocl_mmd_get_offline_info(AOCL_MMD_BOARD_NAMES,
14231443
MAX_BOARD_NAMES_LEN, buf, NULL);
@@ -1487,7 +1507,7 @@ int acl_hal_mmd_try_devices(cl_uint num_devices, const cl_device_id *devices,
14871507
physical_device_id = 0;
14881508

14891509
for (unsigned iboard = 0; iboard < num_board_pkgs; ++iboard) {
1490-
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[iboard];
1510+
acl_mmd_dispatch_t &dispatch = internal_mmd_dispatch[iboard].dispatch;
14911511

14921512
dispatch.aocl_mmd_get_offline_info(AOCL_MMD_BOARD_NAMES,
14931513
MAX_BOARD_NAMES_LEN, buf, NULL);

0 commit comments

Comments
 (0)