From ee9805ccd17670e945e41e62f160bfdd9ad8a386 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 20 Oct 2022 17:59:03 -0400 Subject: cm/filesystem: Fix crash with pre-C++11 std::string GNU ABI in C++17 The `remove_filename` and `replace_extension` methods compute an offset between the whole path in a `std::string` and a part of a path in a `std::string_view`. This is done by subtracting their `.data()` pointers. However, C++17 adds a non-const `.data()` through which modification of the string is allowed. This means the copy-on-write implementation used by the pre-C++11 std::string GNU ABI must reallocate if the string has been copied. Our subtraction then computes an offset between two different allocations, which is undefined behavior. The workaround in commit b3ca4f9ad1 (cm/filesystem: Work around crash when compiled for CYGWIN/MSYS runtime, 2021-04-22, v3.21.0-rc1~271^2~2) avoided the problem by calling the non-const `.data()` to reallocate before constructing the `string_view`. Instead, explicitly call the const `.data()` method on the string, which does not reallocate. Fixes: #22090, #23328 --- Source/Checks/cm_cxx_features.cmake | 4 +--- Source/Checks/cm_cxx_filesystem.cxx | 11 +++++++++++ Utilities/std/cm/filesystem | 16 ++++++---------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/Source/Checks/cm_cxx_features.cmake b/Source/Checks/cm_cxx_features.cmake index f20572e06c..7917d41faf 100644 --- a/Source/Checks/cm_cxx_features.cmake +++ b/Source/Checks/cm_cxx_features.cmake @@ -80,9 +80,7 @@ if(CMake_HAVE_CXX_MAKE_UNIQUE) set(CMake_HAVE_CXX_UNIQUE_PTR 1) endif() cm_check_cxx_feature(unique_ptr) -if (NOT CMAKE_CXX_STANDARD LESS "17" - AND NOT MSYS # FIXME: RunCMake.cmake_path cases crash with MSYS std::filesystem - ) +if (NOT CMAKE_CXX_STANDARD LESS "17") if (NOT CMAKE_CROSSCOMPILING OR CMAKE_CROSSCOMPILING_EMULATOR) cm_check_cxx_feature(filesystem TRY_RUN) else() diff --git a/Source/Checks/cm_cxx_filesystem.cxx b/Source/Checks/cm_cxx_filesystem.cxx index ae8acc56a6..732f28f39e 100644 --- a/Source/Checks/cm_cxx_filesystem.cxx +++ b/Source/Checks/cm_cxx_filesystem.cxx @@ -23,5 +23,16 @@ int main() } #endif + // If std::string is copy-on-write, the std::filesystem::path + // implementation may accidentally trigger a reallocation and compute + // an offset between two allocations, leading to undefined behavior. +#if defined(__GLIBCXX__) && \ + (!defined(_GLIBCXX_USE_CXX11_ABI) || !_GLIBCXX_USE_CXX11_ABI) + std::string p5s1 = "/path"; + std::string p5s2 = std::move(p5s1); + std::filesystem::path p5 = std::string(p5s2); + p5.remove_filename(); +#endif + return 0; } diff --git a/Utilities/std/cm/filesystem b/Utilities/std/cm/filesystem index ce52fbf3b3..b1cb3668f4 100644 --- a/Utilities/std/cm/filesystem +++ b/Utilities/std/cm/filesystem @@ -809,13 +809,11 @@ public: path& remove_filename() { -# if defined(__CYGWIN__) - // FIXME: Avoid crash due to CYGWIN/MSYS bug(?). See CMake Issue 22090. - static_cast(this->path_.data()); -# endif auto fname = this->get_filename(); if (!fname.empty()) { - this->path_.erase(fname.data() - this->path_.data()); + this->path_.erase(fname.data() - + // Avoid C++17 non-const .data() that may reallocate. + static_cast(this->path_).data()); } return *this; } @@ -829,13 +827,11 @@ public: path& replace_extension(const path& replacement = path()) { -# if defined(__CYGWIN__) - // FIXME: Avoid crash due to CYGWIN/MSYS bug(?). See CMake Issue 22090. - static_cast(this->path_.data()); -# endif auto ext = this->get_filename_fragment(filename_fragment::extension); if (!ext.empty()) { - this->path_.erase(ext.data() - this->path_.data()); + this->path_.erase(ext.data() - + // Avoid C++17 non-const .data() that may reallocate. + static_cast(this->path_).data()); } if (!replacement.path_.empty()) { if (replacement.path_[0] != '.') { -- cgit v1.2.1