From 3b8cf64d9e7244984e12afc3fa2f0d9fe4557a86 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Wed, 12 Feb 2020 14:42:16 +0100 Subject: [PATCH 01/10] On Linux, search RUNPATH in addition to LD_LIBRARY_PATH Signed-off-by: Tim Wiese --- src/find_library.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/find_library.cpp b/src/find_library.cpp index b809d47..ff695a1 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include "rcutils/filesystem.h" #include "rcutils/get_env.h" @@ -76,6 +77,27 @@ std::string find_library_path(const std::string & library_name) std::string search_path = get_env_var(kPathVar); std::list search_paths = split(search_path, kPathSeparator); +#if !defined(_WIN32) && !defined(__APPLE__) + std::string search_path_runpath; + + const ElfW(Dyn) *dyn = _DYNAMIC; + const ElfW(Dyn) *runpath = NULL; + const char *strtab = NULL; + + for (; dyn->d_tag != DT_NULL; ++dyn) { + if (dyn->d_tag == DT_RUNPATH) { + runpath = dyn; + } else if (dyn->d_tag == DT_STRTAB) { + strtab = (const char *)dyn->d_un.d_val; + } + } + + if (strtab != NULL && runpath != NULL) { + search_path_runpath = std::string(strtab + runpath->d_un.d_val); + search_paths.splice(search_paths.cend(), split(search_path_runpath, kPathSeparator)); + } +#endif + std::string filename = kSolibPrefix; filename += library_name + kSolibExtension; From 39344688c501f016195941df53669214fea8d7cb Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Wed, 12 Feb 2020 14:44:49 +0100 Subject: [PATCH 02/10] Add test for searching RUNPATH to find library on Linux Signed-off-by: Tim Wiese --- test/test_find_library.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/test_find_library.cpp b/test/test_find_library.cpp index d460904..56defc9 100644 --- a/test/test_find_library.cpp +++ b/test/test_find_library.cpp @@ -59,9 +59,18 @@ TEST(test_find_library, find_library) #endif // Positive test. - const std::string test_lib_actual = find_library_path("test_library"); + std::string test_lib_actual = find_library_path("test_library"); EXPECT_EQ(test_lib_actual, expected_library_path); +#if !defined(_WIN32) && !defined(__APPLE__) + EXPECT_EQ(setenv(env_var, "", override), 0); + + // Test with empty LD_LIBRARY_PATH to emulate setcap / setuid executable. + // Using RUNPATH to find library instead. + test_lib_actual = find_library_path("test_library"); + EXPECT_EQ(test_lib_actual, expected_library_path); +#endif + // (Hopefully) Negative test. const std::string bad_path = find_library_path( "this_is_a_junk_libray_name_please_dont_define_this_if_you_do_then_" From f1d15bb62fdab51dd7b702a3cce2fbbf8f5f3b65 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Wed, 12 Feb 2020 14:50:20 +0100 Subject: [PATCH 03/10] Make sure RUNPATH is set correctly BUILD_RPATH is set to find test library INSTALL_RPATH_USE_LINK_PATH adds linked library paths to RUNPATH Signed-off-by: Tim Wiese --- CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index b6f5414..ec915c5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -34,6 +34,10 @@ if(WIN32) target_compile_definitions(${PROJECT_NAME} PRIVATE "RCPPUTILS_BUILDING_LIBRARY") endif() +set_target_properties(${PROJECT_NAME} PROPERTIES + BUILD_RPATH ${CMAKE_CURRENT_BINARY_DIR} + INSTALL_RPATH_USE_LINK_PATH TRUE +) ament_target_dependencies(${PROJECT_NAME} rcutils) ament_export_libraries(${PROJECT_NAME}) From d4542a3c0f809eb64e9e8ff095dbef9ea17328e4 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Wed, 12 Feb 2020 16:19:32 +0100 Subject: [PATCH 04/10] Fix header order Signed-off-by: Tim Wiese --- src/find_library.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/find_library.cpp b/src/find_library.cpp index ff695a1..b7e6522 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -14,6 +14,8 @@ #include "rcpputils/find_library.hpp" +#include + #include #include @@ -21,7 +23,6 @@ #include #include #include -#include #include "rcutils/filesystem.h" #include "rcutils/get_env.h" From 2b7b0b971eecff89a7b0f490bb147cab774cb41b Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Wed, 12 Feb 2020 16:28:17 +0100 Subject: [PATCH 05/10] Fix pointer code style Signed-off-by: Tim Wiese --- src/find_library.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/find_library.cpp b/src/find_library.cpp index b7e6522..1cae2f9 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -81,9 +81,9 @@ std::string find_library_path(const std::string & library_name) #if !defined(_WIN32) && !defined(__APPLE__) std::string search_path_runpath; - const ElfW(Dyn) *dyn = _DYNAMIC; - const ElfW(Dyn) *runpath = NULL; - const char *strtab = NULL; + const ElfW(Dyn) * dyn = _DYNAMIC; + const ElfW(Dyn) * runpath = NULL; + const char * strtab = NULL; for (; dyn->d_tag != DT_NULL; ++dyn) { if (dyn->d_tag == DT_RUNPATH) { From 71b46d1235323c36b81430a1fe46b568bbda44bd Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Fri, 14 Feb 2020 17:06:00 +0100 Subject: [PATCH 06/10] Put ifdef around include Signed-off-by: Tim Wiese --- src/find_library.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/find_library.cpp b/src/find_library.cpp index 1cae2f9..62b8e5e 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -14,7 +14,10 @@ #include "rcpputils/find_library.hpp" + +#if !defined(_WIN32) && !defined(__APPLE__) #include +#endif #include #include From 0d49a407e6292b51365071388dbb52595c8db0a8 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Fri, 14 Feb 2020 17:07:41 +0100 Subject: [PATCH 07/10] Extract RUNPATH logic into helper method Signed-off-by: Tim Wiese --- include/rcpputils/find_library.hpp | 4 ++-- src/find_library.cpp | 32 ++++++++++++++++++++---------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/include/rcpputils/find_library.hpp b/include/rcpputils/find_library.hpp index 292b984..2f2065d 100644 --- a/include/rcpputils/find_library.hpp +++ b/include/rcpputils/find_library.hpp @@ -23,8 +23,8 @@ namespace rcpputils { /// Finds a library located in the OS's specified environment variable for -/// library paths and returns the absolute filesystem path, including the -/// appropriate prefix and extension. +/// library paths or in RUNPATH on Linux and returns the absolute filesystem +/// path, including the appropriate prefix and extension. /** * The environment variable and file format per platform: * * Linux: `${LD_LIBRARY_PATH}`, `lib{}.so` diff --git a/src/find_library.cpp b/src/find_library.cpp index 62b8e5e..74ca142 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -74,16 +74,11 @@ std::list split(const std::string & value, const char delimiter) return list; } -} // namespace - -std::string find_library_path(const std::string & library_name) -{ - std::string search_path = get_env_var(kPathVar); - std::list search_paths = split(search_path, kPathSeparator); - #if !defined(_WIN32) && !defined(__APPLE__) - std::string search_path_runpath; - +// Retrieves a list of paths from the RUNPATH header on Linux. +// Useful when LD_LIBRARY_PATH is ignored for setcap / setuid executables. +std::list retrieve_runpath_paths() +{ const ElfW(Dyn) * dyn = _DYNAMIC; const ElfW(Dyn) * runpath = NULL; const char * strtab = NULL; @@ -96,10 +91,25 @@ std::string find_library_path(const std::string & library_name) } } + std::string search_path; if (strtab != NULL && runpath != NULL) { - search_path_runpath = std::string(strtab + runpath->d_un.d_val); - search_paths.splice(search_paths.cend(), split(search_path_runpath, kPathSeparator)); + search_path = std::string(strtab + runpath->d_un.d_val); } + + return split(search_path, kPathSeparator); +} +#endif + +} // namespace + +std::string find_library_path(const std::string & library_name) +{ + std::string search_path = get_env_var(kPathVar); + std::list search_paths = split(search_path, kPathSeparator); + +#if !defined(_WIN32) && !defined(__APPLE__) + std::list search_paths_runpath = retrieve_runpath_paths(); + search_paths.splice(search_paths.cend(), search_paths_runpath); #endif std::string filename = kSolibPrefix; From f4ee23846b685cfa981df2f8cb714e0ca6fb3bd6 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Tue, 18 Feb 2020 11:16:35 +0100 Subject: [PATCH 08/10] Return empty list in RUNPATH search on win / macos Signed-off-by: Tim Wiese --- src/find_library.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/find_library.cpp b/src/find_library.cpp index 74ca142..b78d75a 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -14,7 +14,6 @@ #include "rcpputils/find_library.hpp" - #if !defined(_WIN32) && !defined(__APPLE__) #include #endif @@ -74,11 +73,14 @@ std::list split(const std::string & value, const char delimiter) return list; } -#if !defined(_WIN32) && !defined(__APPLE__) // Retrieves a list of paths from the RUNPATH header on Linux. // Useful when LD_LIBRARY_PATH is ignored for setcap / setuid executables. std::list retrieve_runpath_paths() { +#if defined(_WIN32) || defined(__APPLE__) + // Return empty list for win / macos + return std::list(); +#else const ElfW(Dyn) * dyn = _DYNAMIC; const ElfW(Dyn) * runpath = NULL; const char * strtab = NULL; @@ -97,20 +99,18 @@ std::list retrieve_runpath_paths() } return split(search_path, kPathSeparator); -} #endif +} } // namespace std::string find_library_path(const std::string & library_name) { - std::string search_path = get_env_var(kPathVar); - std::list search_paths = split(search_path, kPathSeparator); + std::string search_path_env = get_env_var(kPathVar); + std::list search_paths = split(search_path_env, kPathSeparator); -#if !defined(_WIN32) && !defined(__APPLE__) std::list search_paths_runpath = retrieve_runpath_paths(); search_paths.splice(search_paths.cend(), search_paths_runpath); -#endif std::string filename = kSolibPrefix; filename += library_name + kSolibExtension; From c12ae09740b93e83c642e9f583bf8c24a335d036 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Mon, 24 Feb 2020 14:16:58 +0100 Subject: [PATCH 09/10] Set library path env var through cmake test properties and reuse find_library test for RUNPATH test case This should avoid unintended effects of global environment changes, as cmake's set_tests_properties restores the environment to its previous state after the test is done. It also emulates better the effect of a setcap / setuid executable for the RUNPATH test case (LD_LIBRARY_PATH being dropped at execution). Signed-off-by: Tim Wiese --- CMakeLists.txt | 21 ++++++++++++++++++++- test/test_find_library.cpp | 30 ------------------------------ 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ec915c5..ec7ae63 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,12 +75,31 @@ if(BUILD_TESTING) ament_add_gtest(test_endian test/test_endian.cpp) + if(WIN32) + set(LIBRARY_PATH_VAR "PATH") + elseif(APPLE) + set(LIBRARY_PATH_VAR "DYLD_LIBRARY_PATH") + else() + set(LIBRARY_PATH_VAR "LD_LIBRARY_PATH") + endif() + add_library(test_library SHARED test/test_library.cpp) + ament_add_gtest(test_find_library test/test_find_library.cpp) target_link_libraries(test_find_library ${PROJECT_NAME} test_library) set_tests_properties(test_find_library PROPERTIES ENVIRONMENT - "_TEST_LIBRARY_DIR=$;_TEST_LIBRARY=$") + "${LIBRARY_PATH_VAR}=$;_TEST_LIBRARY=$") + + # Test with empty LD_LIBRARY_PATH to emulate setcap / setuid executable. + # Using RUNPATH to find library instead. + if(NOT (WIN32 OR APPLE)) + ament_add_gtest(test_find_library_runpath test/test_find_library.cpp) + target_link_libraries(test_find_library_runpath ${PROJECT_NAME} test_library) + set_tests_properties(test_find_library_runpath PROPERTIES + ENVIRONMENT + "${LIBRARY_PATH_VAR}=;_TEST_LIBRARY=$") + endif() endif() ament_package() diff --git a/test/test_find_library.cpp b/test/test_find_library.cpp index 56defc9..464418c 100644 --- a/test/test_find_library.cpp +++ b/test/test_find_library.cpp @@ -37,40 +37,10 @@ TEST(test_find_library, find_library) expected_library_path = _expected_library_path; } - const char * test_lib_dir{}; - EXPECT_EQ(rcutils_get_env("_TEST_LIBRARY_DIR", &test_lib_dir), nullptr); - EXPECT_NE(test_lib_dir, nullptr); - - // Set our relevant path variable. - const char * env_var{}; -#ifdef _WIN32 - env_var = "PATH"; -#elif __APPLE__ - env_var = "DYLD_LIBRARY_PATH"; -#else - env_var = "LD_LIBRARY_PATH"; -#endif - -#ifdef _WIN32 - EXPECT_EQ(_putenv_s(env_var, test_lib_dir), 0); -#else - const int override = 1; - EXPECT_EQ(setenv(env_var, test_lib_dir, override), 0); -#endif - // Positive test. std::string test_lib_actual = find_library_path("test_library"); EXPECT_EQ(test_lib_actual, expected_library_path); -#if !defined(_WIN32) && !defined(__APPLE__) - EXPECT_EQ(setenv(env_var, "", override), 0); - - // Test with empty LD_LIBRARY_PATH to emulate setcap / setuid executable. - // Using RUNPATH to find library instead. - test_lib_actual = find_library_path("test_library"); - EXPECT_EQ(test_lib_actual, expected_library_path); -#endif - // (Hopefully) Negative test. const std::string bad_path = find_library_path( "this_is_a_junk_libray_name_please_dont_define_this_if_you_do_then_" From f691a10940eefa9d7431f10fa3bd1add35d0f3d0 Mon Sep 17 00:00:00 2001 From: Tim Wiese Date: Mon, 16 Mar 2020 16:17:17 +0100 Subject: [PATCH 10/10] Add const qualifier Co-Authored-By: Thomas Moulard Signed-off-by: Tim Wiese --- src/find_library.cpp | 2 +- test/test_find_library.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/find_library.cpp b/src/find_library.cpp index b78d75a..d536d75 100644 --- a/src/find_library.cpp +++ b/src/find_library.cpp @@ -106,7 +106,7 @@ std::list retrieve_runpath_paths() std::string find_library_path(const std::string & library_name) { - std::string search_path_env = get_env_var(kPathVar); + const std::string search_path_env = get_env_var(kPathVar); std::list search_paths = split(search_path_env, kPathSeparator); std::list search_paths_runpath = retrieve_runpath_paths(); diff --git a/test/test_find_library.cpp b/test/test_find_library.cpp index 464418c..0e22452 100644 --- a/test/test_find_library.cpp +++ b/test/test_find_library.cpp @@ -38,7 +38,7 @@ TEST(test_find_library, find_library) } // Positive test. - std::string test_lib_actual = find_library_path("test_library"); + const std::string test_lib_actual = find_library_path("test_library"); EXPECT_EQ(test_lib_actual, expected_library_path); // (Hopefully) Negative test.