From 0f91901446cf1cc1beece4f824e8d85a0eea5ddd Mon Sep 17 00:00:00 2001 From: John Parent Date: Fri, 30 Jan 2026 19:02:26 -0500 Subject: [PATCH 1/7] Relocation: Change strategy Previously we were searching a set of prefixes for DLLs during relocation. If we found a dll that matched the dll we were looking for (based on file name) we performed relocation based on that path. This is both dangerous and extraneous. This is dangerous as mutli config layouts may have the same binary with the same name in mutliple different paths for different configs or variations. Since we were previously only checking the filename, this could lead to a false positive detection and bad relocation not detected until runtime. This is extraneous as we should never need to search. We have the dll locations before and after relocation, whether from the stage to install prefix or from buildcache to buildcache, so rather than a filesystem search, we can have a linear time operation where we search through a list of relocation old->new prefix mappings. Spack core will set an environment variable of the structure: old_prefix|new_prefix and the compiler wrapper now composes a map out of that list and then PE files looking to relocate their internal dll references get a constant time lookup. Signed-off-by: John Parent --- Makefile | 70 ++++++++++++++++++++--------------- src/utils.cxx | 63 ++++++++++++++++++++++++++++++- src/utils.h | 14 +++++++ test/setup_and_drive_test.bat | 1 - 4 files changed, 117 insertions(+), 31 deletions(-) diff --git a/Makefile b/Makefile index 228240d..4f81102 100644 --- a/Makefile +++ b/Makefile @@ -72,9 +72,10 @@ install : cl.exe mklink $(PREFIX)\relocate.exe $(PREFIX)\cl.exe setup_test: cl.exe - echo "-------------------" - echo "Running Test Setup" - echo "-------------------" + @echo \n + @echo ------------------- + @echo Running Test Setup + @echo ------------------- -@ if NOT EXIST "tmp\test" mkdir "tmp\test" cd tmp\test copy ..\..\cl.exe cl.exe @@ -86,9 +87,9 @@ setup_test: cl.exe # * space in a path - preserved by quoted arguments # * escaped quoted arguments build_and_check_test_sample : setup_test - echo "--------------------" - echo "Building Test Sample" - echo "--------------------" + @echo -------------------- + @echo Building Test Sample + @echo -------------------- cd tmp\test cl /c /EHsc "..\..\test\src file\calc.cxx" /DCALC_EXPORTS /DCALC_HEADER="\"calc header/calc.h\"" /I ..\..\test\include cl /c /EHsc ..\..\test\main.cxx /I ..\..\test\include @@ -100,9 +101,10 @@ build_and_check_test_sample : setup_test # Test basic wrapper behavior - did the absolute path to the DLL get injected # into the executable test_wrapper : build_and_check_test_sample - echo "--------------------" - echo "Running Wrapper Test" - echo "--------------------" + @echo \n + @echo -------------------- + @echo Running Wrapper Test + @echo -------------------- cd tmp move test\tester.exe .\tester.exe .\tester.exe @@ -116,23 +118,25 @@ test_wrapper : build_and_check_test_sample # Test relocating an executable - re-write internal paths to dlls test_relocate_exe: build_and_check_test_sample - echo "--------------------------" - echo "Running Relocate Exe Test" - echo "--------------------------" + @echo \n + @echo -------------------------- + @echo Running Relocate Exe Test + @echo -------------------------- cd tmp\test -@ if NOT EXIST "relocate.exe" mklink relocate.exe cl.exe move calc.dll ..\calc.dll - relocate.exe --pe tester.exe --deploy --full - relocate.exe --pe tester.exe --export --full + SET SPACK_RELOCATE_PATH=$(MAKEDIR)\tmp\test\calc.dll|$(MAKEDIR)\tmp\calc.dll + relocate.exe --pe tester.exe --full tester.exe move ..\calc.dll calc.dll cd ../.. # Test relocating a dll - re-write import library test_relocate_dll: build_and_check_test_sample - echo "--------------------------" - echo "Running Relocate DLL test" - echo "--------------------------" + @echo \n + @echo -------------------------- + @echo Running Relocate DLL test + @echo -------------------------- cd tmp/test -@ if NOT EXIST "relocate.exe" mklink relocate.exe cl.exe cd .. @@ -171,18 +175,20 @@ build_zerowrite_test: test\writezero.obj link $(LFLAGS) $** $(API_LIBS) /out:writezero.exe test_zerowrite: build_zerowrite_test - echo "-----------------------" - echo "Running zerowrite test" - echo "-----------------------" + @echo \n + @echo ----------------------- + @echo Running zerowrite test + @echo ----------------------- set SPACK_CC_TMP=%SPACK_CC% set SPACK_CC=$(MAKEDIR)\writezero.exe cl /c EHsc "test\src file\calc.cxx" set SPACK_CC=%SPACK_CC_TMP% test_long_paths: build_and_check_test_sample - echo "------------------------" - echo "Running long paths test" - echo "------------------------" + @echo \n + @echo ------------------------ + @echo Running long paths test + @echo ------------------------ mkdir tmp\tmp\verylongdirectoryname\evenlongersubdirectoryname xcopy /E test\include tmp\tmp\verylongdirectoryname\evenlongersubdirectoryname xcopy /E "test\src file" tmp\tmp\verylongdirectoryname\evenlongersubdirectoryname @@ -199,9 +205,10 @@ test_long_paths: build_and_check_test_sample cd ../../../.. test_relocate_long_paths: test_long_paths - echo "---------------------------------" - echo "Running relocate logn paths test" - echo "---------------------------------" + @echo \n + @echo --------------------------------- + @echo Running relocate logn paths test + @echo --------------------------------- cd tmp\tmp\verylongdirectoryname\evenlongersubdirectoryname -@ if NOT EXIST "relocate.exe" mklink relocate.exe cl.exe cd .. @@ -217,9 +224,10 @@ test_relocate_long_paths: test_long_paths cd ../../../.. test_exe_with_exports: - echo ------------------------------ - echo Running exe with exports test - echo ------------------------------ + @echo \n + @echo ------------------------------ + @echo Running exe with exports test + @echo ------------------------------ mkdir tmp\test\exe_with_exports xcopy /E test\include tmp\test\exe_with_exports xcopy /E "test\src file" tmp\test\exe_with_exports @@ -240,6 +248,10 @@ test_exe_with_exports: cd ../../.. test_def_file_name_override: + @echo + @echo ------------------------------------ + @echo Running Def file name override test + @echo ------------------------------------ mkdir tmp\test\def\def_override xcopy /E test\include tmp\test\def\def_override xcopy /E "test\src file" tmp\test\def\def_override diff --git a/src/utils.cxx b/src/utils.cxx index 25809b4..52f323d 100644 --- a/src/utils.cxx +++ b/src/utils.cxx @@ -875,13 +875,74 @@ std::string LibraryFinder::Finder(const std::string& pth, return std::string(); } +PathRelocator::PathRelocator() { + this->new_prefix_ = GetSpackEnv("SPACK_INSTALL_PREFIX"); + this->parseRelocate(); +} + +void PathRelocator::parseRelocate() { + const std::string relocations = GetSpackEnv("SPACK_RELOCATE_PATH"); + // relocations is a semi colon separated list of + // | separated pairs, of old_prefix|new_prefix + // where old prefix is either the stage or the + // old install root and new prefix is the dll location in the + // install tree or just the new install prefix + if (relocations.empty()) { + return; + } + const StrList mappings = split(relocations, ";"); + for (const auto& pair : mappings) { + const StrList old_new = split(pair, "|"); + const std::string& old = old_new[0]; + const std::string& new_ = old_new[1]; + this->old_new_map[old] = new_; + if (endswith(old, ".dll") || endswith(old, ".exe")) { + this->bc_ = false; + } + } +} + +std::string PathRelocator::getRelocation(std::string const& pe) { + if (this->bc_) { + return this->relocateBC(pe); + } + return this->relocateStage(pe); +} + +std::string PathRelocator::relocateBC(std::string const& pe) { + for (auto& root : this->old_new_map) { + if (startswith(pe, root.first)) { + std::array rel_root; + if (PathRelativePathToW( + &rel_root[0], ConvertASCIIToWide(root.first).c_str(), + FILE_ATTRIBUTE_DIRECTORY, ConvertASCIIToWide(pe).c_str(), + FILE_ATTRIBUTE_NORMAL) != 0) { + // we have the pe's relative root in the old + // prefix, slap the new prefix on it and return + std::string const real_rel( + ConvertWideToASCII(std::wstring(&rel_root[0]))); + return join({root.second, real_rel}, "\\"); + } + } + } + return std::string(); +} + +std::string PathRelocator::relocateStage(std::string const& pe) { + try { + std::string prefix_loc = this->old_new_map.at(pe); + return prefix_loc; + } catch (std::out_of_range& e) { + return std::string(); + } +} + namespace { std::vector system_locations = { "api-ms-", "ext-ms-", "ieshims", "emclient", "devicelock", "wpax", "vcruntime", "WINDOWS", "system32", "KERNEL32", "WS2_32", "dbghelp", "bcrypt", "ADVAPI32", "SHELL32", "CRYPT32", "USER32", "ole32", "OLEAUTH32"}; - } bool LibraryFinder::IsSystem(const std::string& pth) { diff --git a/src/utils.h b/src/utils.h index f0eb71b..f738662 100644 --- a/src/utils.h +++ b/src/utils.h @@ -293,6 +293,20 @@ class LibraryFinder { void EvalSearchPaths(); }; +class PathRelocator { + private: + bool bc_; + std::string new_prefix_; + std::map old_new_map; + std::string relocateBC(std::string const& pe); + std::string relocateStage(std::string const& pe); + void parseRelocate(); + + public: + PathRelocator(); + std::string getRelocation(std::string const& pe); +}; + using ScopedLocalInfo = std::unique_ptr; using ScopedSid = std::unique_ptr; diff --git a/test/setup_and_drive_test.bat b/test/setup_and_drive_test.bat index 88285fd..2ae5e83 100644 --- a/test/setup_and_drive_test.bat +++ b/test/setup_and_drive_test.bat @@ -17,6 +17,5 @@ SET SPACK_DEBUG_LOG_ID=TEST SET SPACK_SHORT_SPEC=test%msvc SET SPACK_SYSTEM_DIRS=%PATH% SET SPACK_MANAGED_DIRS=%CD%\tmp -SET SPACK_RELOCATE_PATH=%CD%\tmp nmake test \ No newline at end of file From ef03c5df979f886c9579e34de77fcb91552f2a1d Mon Sep 17 00:00:00 2001 From: John Parent Date: Thu, 9 Apr 2026 19:17:41 -0400 Subject: [PATCH 2/7] add array header Signed-off-by: John Parent --- src/utils.cxx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/utils.cxx b/src/utils.cxx index 52f323d..d2eac6c 100644 --- a/src/utils.cxx +++ b/src/utils.cxx @@ -39,6 +39,7 @@ #include #include #include +#include #include "shlwapi.h" #include "PathCch.h" From 5007911733cb03ba3ba0cb9f5f92379460c4a45f Mon Sep 17 00:00:00 2001 From: John Parent Date: Fri, 30 Jan 2026 19:14:53 -0500 Subject: [PATCH 3/7] RPathing Logic Overhaul * Makes all PE paths absolute * Removes rename logic that dealt with the spack sigil and BC pushes * Uses new relocation logic to avoid FS search and instead use Spack env variable and util support added in prior PR * Adds access rights scoping for read and write operations * General code cleanup Signed-off-by: John Parent --- src/winrpath.cxx | 73 +++++++++++++++++++++++++----------------------- src/winrpath.h | 6 ++-- 2 files changed, 41 insertions(+), 38 deletions(-) diff --git a/src/winrpath.cxx b/src/winrpath.cxx index 1249e3e..400ba72 100644 --- a/src/winrpath.cxx +++ b/src/winrpath.cxx @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -35,7 +36,7 @@ * \param name The dll name to check for path characters * */ -bool LibRename::SpackCheckForDll(const std::string& dll_path) const { +bool LibRename::SpackCheckForDll(const std::string& dll_path) { return hasPathCharacters(dll_path); } @@ -50,38 +51,28 @@ bool LibRename::SpackCheckForDll(const std::string& dll_path) const { * the dll name found at `name_loc` to the absolute path of * */ -bool LibRename::RenameDll(char* name_loc, const std::string& dll_path) const { +bool LibRename::RenameDll(char* name_loc, const std::string& dll_path) { if (SpackInstalledLib(dll_path)) { return true; } - std::string const file_name = basename(dll_path); - if (file_name.empty()) { - std::cerr << "Unable to extract filename from dll for relocation" - << "\n"; + PathRelocator relocator; + std::string new_loc = relocator.getRelocation(dll_path); + if (new_loc.empty()) { + std::cerr << "Cannot find relocation mapping for library " << dll_path + << "\n"; return false; } - LibraryFinder lib_finder; - std::string new_library_loc = - lib_finder.FindLibrary(file_name, dll_path); - if (new_library_loc.empty()) { - std::cerr << "Unable to find library " << file_name << " from " - << dll_path << " for relocation" << "\n"; + try { + new_loc = + EnsureValidLengthPath(CanonicalizePath(MakePathAbsolute(new_loc))); + } catch (NameTooLongError& e) { + std::cerr << "Cannot relocate path " << new_loc + << "it is too long to be relocated safely.\n"; return false; } - if (new_library_loc.length() > MAX_NAME_LEN) { - try { - new_library_loc = short_name(new_library_loc); - } catch (NameTooLongError& e) { - return false; - } catch (FileNotExist &e) { - return false; - } catch (SFNProcessingError &e) { - return false; - } - } + char* new_lib_pth = - pad_path(new_library_loc.c_str(), - static_cast(new_library_loc.size())); + pad_path(new_loc.c_str(), static_cast(new_loc.size())); if (!new_lib_pth) { return false; } @@ -176,8 +167,8 @@ bool LibRename::FindDllAndRename(HANDLE& pe_in) { import_table_offset + (import_image_descriptor->Name - rva_import_directory); std::string const str_dll_name = std::string(imported_dll); - if (this->SpackCheckForDll(str_dll_name)) { - if (!this->RenameDll(imported_dll, str_dll_name)) { + if (LibRename::SpackCheckForDll(str_dll_name)) { + if (!LibRename::RenameDll(imported_dll, str_dll_name)) { std::cerr << "Unable to relocate DLL reference: " << str_dll_name << "\n"; return false; @@ -209,6 +200,7 @@ bool LibRename::FindDllAndRename(HANDLE& pe_in) { */ LibRename::LibRename(std::string p_exe, bool full, bool replace) : replace(replace), full(full), pe(std::move(p_exe)) { + this->pe = MakePathAbsolute(this->pe); } LibRename::LibRename(std::string p_exe, std::string coff, bool full, @@ -217,6 +209,7 @@ LibRename::LibRename(std::string p_exe, std::string coff, bool full, full(full), pe(std::move(p_exe)), coff(std::move(coff)) { + this->pe = MakePathAbsolute(this->pe); std::string const coff_path = stem(this->coff); this->tmp_def_file = coff_path + "-tmp.def"; this->def_file = coff_path + ".def"; @@ -287,7 +280,9 @@ bool LibRename::ComputeDefFile() { npos) { // Skip header in export block if still present break; } - output_file << " " << regexReplace(line, R"(\s+)", "") << '\n'; + output_file << " " + << regexMatch(line, R"(^.*?(\S+)(?:\s+\(.*\))?\s*$)") + << '\n'; } input_file.close(); output_file.close(); @@ -402,15 +397,23 @@ bool LibRename::ExecutePERename() { std::cerr << e.what() << "\n"; return false; } - HANDLE pe_handle = CreateFileW( - pe_path.c_str(), (GENERIC_READ | GENERIC_WRITE), FILE_SHARE_READ, - nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); - if (!pe_handle || pe_handle == INVALID_HANDLE_VALUE) { - std::cerr << "Unable to acquire file handle to " << pe_path.c_str() - << ": " << reportLastError() << "\n"; + try { + ScopedFileAccess const obtain_write(pe_path, GENERIC_ALL); + HANDLE pe_handle = CreateFileW( + pe_path.c_str(), (GENERIC_READ | GENERIC_WRITE), FILE_SHARE_WRITE, + nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + if (!pe_handle || pe_handle == INVALID_HANDLE_VALUE) { + std::cerr << "Unable to acquire file handle to " + << ConvertWideToASCII(pe_path) << ": " + << reportLastError() << "\n"; + return false; + } + return LibRename::FindDllAndRename(pe_handle); + } catch (const std::system_error& e) { + std::cerr << "Could not obtain write access: " << e.what() + << " (Error Code: " << e.code().value() << ")" << '\n'; return false; } - return this->FindDllAndRename(pe_handle); } /* Construct the line needed to produce a new import library diff --git a/src/winrpath.h b/src/winrpath.h index 0e5061f..16aeff6 100644 --- a/src/winrpath.h +++ b/src/winrpath.h @@ -41,9 +41,9 @@ class LibRename { std::string ComputeDefLine(); private: - bool FindDllAndRename(HANDLE& pe_in); - bool SpackCheckForDll(const std::string& dll_path) const; - bool RenameDll(char* name_loc, const std::string& dll_path) const; + static bool FindDllAndRename(HANDLE& pe_in); + static bool SpackCheckForDll(const std::string& dll_path) ; + static bool RenameDll(char* name_loc, const std::string& dll_path) ; ExecuteCommand def_executor; ExecuteCommand lib_executor; std::string pe; From f1760b0bc3ed5e8c6c75ce7b3031dfe2ca2bb0dc Mon Sep 17 00:00:00 2001 From: John Parent Date: Thu, 23 Apr 2026 19:40:45 -0400 Subject: [PATCH 4/7] cleanup after rebase Signed-off-by: John Parent --- src/utils.cxx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/utils.cxx b/src/utils.cxx index d2eac6c..567bd79 100644 --- a/src/utils.cxx +++ b/src/utils.cxx @@ -344,7 +344,7 @@ std::smatch regexSearch( return match; } -std::smatch regexMatch( +std::string regexMatch( const std::string& searchDomain, const std::string& regex, const std::vector& opts, const std::vector& flags) { @@ -354,9 +354,11 @@ std::smatch regexMatch( std::regex const reg(regex, opt); std::smatch match; if (!std::regex_match(searchDomain, match, reg, flag)) { - return std::smatch(); + return std::string(); + } + else { + return match.str(1); } - return match; } std::string regexReplace( From 60cb9a81e7a24a5884d82f64609b78f6e0258b57 Mon Sep 17 00:00:00 2001 From: John Parent Date: Thu, 23 Apr 2026 19:42:26 -0400 Subject: [PATCH 5/7] doh Signed-off-by: John Parent --- src/utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils.h b/src/utils.h index f738662..76ff1c6 100644 --- a/src/utils.h +++ b/src/utils.h @@ -160,7 +160,7 @@ std::smatch regexSearch( /// @param regex - regex used to match /// @param opts - optional argument, list of regex tuning options to adapt the match behavior /// @return Character sequence matching regex -std::smatch regexMatch( +std::string regexMatch( const std::string& searchDomain, const std::string& regex, const std::vector& opts = {}, const std::vector& flags = {}); From 1f74b238dda02ef2c349021bf1e2f81073fc8de0 Mon Sep 17 00:00:00 2001 From: John Parent Date: Mon, 27 Apr 2026 17:54:35 -0400 Subject: [PATCH 6/7] relocate on padded rename Signed-off-by: John Parent --- src/utils.cxx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/utils.cxx b/src/utils.cxx index 567bd79..85b1b70 100644 --- a/src/utils.cxx +++ b/src/utils.cxx @@ -906,10 +906,11 @@ void PathRelocator::parseRelocate() { } std::string PathRelocator::getRelocation(std::string const& pe) { + std::string unpadded_pe = strip_padding(pe); if (this->bc_) { - return this->relocateBC(pe); + return this->relocateBC(unpadded_pe); } - return this->relocateStage(pe); + return this->relocateStage(unpadded_pe); } std::string PathRelocator::relocateBC(std::string const& pe) { From 45d986551de04873bd01ef23d48d17b6ef71c362 Mon Sep 17 00:00:00 2001 From: John Parent Date: Mon, 27 Apr 2026 18:21:09 -0400 Subject: [PATCH 7/7] Correct regex match and search caller behavior Signed-off-by: John Parent --- src/utils.cxx | 8 +++----- src/utils.h | 2 +- src/winrpath.cxx | 4 ++-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/utils.cxx b/src/utils.cxx index 85b1b70..baa3004 100644 --- a/src/utils.cxx +++ b/src/utils.cxx @@ -344,7 +344,7 @@ std::smatch regexSearch( return match; } -std::string regexMatch( +std::smatch regexMatch( const std::string& searchDomain, const std::string& regex, const std::vector& opts, const std::vector& flags) { @@ -354,11 +354,9 @@ std::string regexMatch( std::regex const reg(regex, opt); std::smatch match; if (!std::regex_match(searchDomain, match, reg, flag)) { - return std::string(); - } - else { - return match.str(1); + return std::smatch(); } + return match; } std::string regexReplace( diff --git a/src/utils.h b/src/utils.h index 76ff1c6..f738662 100644 --- a/src/utils.h +++ b/src/utils.h @@ -160,7 +160,7 @@ std::smatch regexSearch( /// @param regex - regex used to match /// @param opts - optional argument, list of regex tuning options to adapt the match behavior /// @return Character sequence matching regex -std::string regexMatch( +std::smatch regexMatch( const std::string& searchDomain, const std::string& regex, const std::vector& opts = {}, const std::vector& flags = {}); diff --git a/src/winrpath.cxx b/src/winrpath.cxx index 400ba72..fbdb45f 100644 --- a/src/winrpath.cxx +++ b/src/winrpath.cxx @@ -265,7 +265,7 @@ bool LibRename::ComputeDefFile() { // Read until the output column titles while (std::getline(input_file, line)) { std::smatch search_res = regexSearch(line, R"(ordinal\s+name)"); - if (search_res.empty()) break; + if (!search_res.empty()) break; std::string const res = search_res.str(); if (!res.empty()) { break; @@ -281,7 +281,7 @@ bool LibRename::ComputeDefFile() { break; } output_file << " " - << regexMatch(line, R"(^.*?(\S+)(?:\s+\(.*\))?\s*$)") + << regexMatch(line, R"(^.*?(\S+)(?:\s+\(.*\))?\s*$)").str(1) << '\n'; } input_file.close();