Fixed coverity issues in acl_hal_mmd.cpp

[haoxian2]

Fixed coverity issues in acl_hal_mmd.cpp: Type: 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 struct as a global initialized wrapper that would be deallocated when closing the runtime library. This wrapper would contain all the opened libraries in a vector that would be closed by the destructor of dl_wrapper_t before the vector deallocates.

[haoxian2]

In the original code, there is this if-statement starting at line 581:

auto *test_symbol =
      my_dlsym(mmd_library, "aocl_mmd_get_offline_info", &error_msg);
  if (!test_symbol) {
    // On Linux, for custom libraries close the library (which was opened
    // locally) and then reopen globally. For Windows, there is no option (i.e.
    // it is always global)
#ifdef __linux__
    my_dlclose(mmd_library);
    ACL_HAL_DEBUG_MSG_VERBOSE(
        1, "This library is a custom library. Opening globally.\n");
    mmd_library = my_dlopen_global(library_name, &error_msg);
    if (!mmd_library) {
      std::cout << "Error: Could not load custom library " << library_name;
      if (error_msg && error_msg[0] != '\0') {
        std::cout << " (error_msg: " << error_msg << ")";
      }
      std::cout << "\n";
      return CL_FALSE;
    }
#endif
  } else {
    if (load_libraries) {
      # ....... load the library .......
    }
    ++num_boards_found;
  }
return CL_TRUE;

As you can see, in the even that !test_symbol is true, we do mmd_library = my_dlopen_global(library_name, &error_msg), but we are not doing anything with this opened library since after it is allocated, it falls off of the if-statement and the function immediately returns. Do you know what this block is intended to do?

[pcolberg]

Closing the library invalidates the symbol return by dlsym(), which means calling the function further down is undefined behaviour. These dlopen()-related leaks are likely nontrivial to fix and require a redesign. You could start with a RAII-safe wrapper, e.g.,

struct dl_wrapper {
  dl_wrapper(...) { this->handle = dlopen(...); }
  ~dl_wrapper() { dlclose(this->handle); }
  // prohibit copying to avoid double-close of handle
  dl_wrapper(const dl_wrapper &) = delete;
  dl_wrapper &operator=(const dl_wrapper &) = delete;
  void *sym(...) { return dlsym(this->handle, ...); }
};

The nontrivial part is where to store the wrapper object to ensure the symbols remain valid over the lifetime of their intended use. Maybe there is an easier solution to avoid the resource leak?

[haoxian2]

The solution I proposes in this PR is to keep track of the opened libraries with a wrapper of a vector of mmd_libs in the global scope. The wrapper would be similar to the one you proposed. The vector would deallocate with the closing of the runtime library, and all the libraries present inside will close themselves through the destructor.

I got this vector idea from std::vector<acl_mmd_dispatch_t> internal_mmd_dispatch. This vector is where all the symbols end up going and is declared as a global vector in the .cpp file.

[pcolberg]

Thanks @haoxian2, this is going in the right direction. The RAII wrapper should fully encapsulate the management of the resource; ideally, the constructor acquires the resource, currently my_dlopen(), and the destructor releases the resource, currently my_dlclose().

However, the situation here is slightly more complicated by the presence of two methods to load the library, my_dlopen() versus my_dlopen_global(). Let me take a detailed look at this issue before working out a solution.