From d0ab8ee9a290f64d589836afd16e2e7b28d6ad81 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 13:04:07 +0100 Subject: [PATCH 1/5] Fix segfaults caused by modifying existing shared library When the built shared library is copied to the correct location, it actually *modifies* any existing shared library, which causes any running process using it to segfault. To fix this, we first delete any existing shared library (which is safe to do) and then copy the newly built shared library to the correct location as a new file. Related to #257 --- setuptools_rust/build.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/setuptools_rust/build.py b/setuptools_rust/build.py index 21c19758..9b84bf94 100644 --- a/setuptools_rust/build.py +++ b/setuptools_rust/build.py @@ -345,6 +345,26 @@ def install_extension( os.makedirs(os.path.dirname(ext_path), exist_ok=True) log.info("Copying rust artifact from %s to %s", dylib_path, ext_path) + + # We delete any existing library file before copying to avoid + # causing running processes that use it from segfaulting. + # + # This is because shared libraries are memory mapped into processes + # that use it, so modifying the shared library file (which is what + # `shutil.copyfile` does) means the next time a process calls into + # the shared library they get the updated library rather than the + # original one, causing confusion and segfaults. + # + # Deleting a memory mapped file, on the other hand, is correctly + # handled by the OS, i.e. a copy is kept around until the running + # process exits. Thus, deleting the existing library file and *then* + # copying the new one won't change the existing memory mapped file, + # and so is safe to do. + try: + os.remove(ext_path) + except FileNotFoundError: + pass + shutil.copyfile(dylib_path, ext_path) if sys.platform != "win32" and not debug_build: From 2bb9ba741e1ad44c6817885d25dbd38de96bfbce Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 17:38:21 +0100 Subject: [PATCH 2/5] Use os.rename --- setuptools_rust/build.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/setuptools_rust/build.py b/setuptools_rust/build.py index 9b84bf94..242d46a3 100644 --- a/setuptools_rust/build.py +++ b/setuptools_rust/build.py @@ -351,7 +351,7 @@ def install_extension( # # This is because shared libraries are memory mapped into processes # that use it, so modifying the shared library file (which is what - # `shutil.copyfile` does) means the next time a process calls into + # copying the file does) means the next time a process calls into # the shared library they get the updated library rather than the # original one, causing confusion and segfaults. # @@ -365,7 +365,7 @@ def install_extension( except FileNotFoundError: pass - shutil.copyfile(dylib_path, ext_path) + os.rename(dylib_path, ext_path) if sys.platform != "win32" and not debug_build: args = [] From 78b615f56ee1796714bd982d520847d76bc80a7e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 18:16:02 +0100 Subject: [PATCH 3/5] Use os.replace --- setuptools_rust/build.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/setuptools_rust/build.py b/setuptools_rust/build.py index 242d46a3..f498395a 100644 --- a/setuptools_rust/build.py +++ b/setuptools_rust/build.py @@ -346,26 +346,17 @@ def install_extension( log.info("Copying rust artifact from %s to %s", dylib_path, ext_path) - # We delete any existing library file before copying to avoid - # causing running processes that use it from segfaulting. + # We want to atomically replace any existing library file. We can't + # just copy the new library directly on top of the old one as that + # causes the existing library to be modified (rather the replaced). + # This means that any process that currently uses the shared library + # will see it modified and likely segfault. # - # This is because shared libraries are memory mapped into processes - # that use it, so modifying the shared library file (which is what - # copying the file does) means the next time a process calls into - # the shared library they get the updated library rather than the - # original one, causing confusion and segfaults. - # - # Deleting a memory mapped file, on the other hand, is correctly - # handled by the OS, i.e. a copy is kept around until the running - # process exits. Thus, deleting the existing library file and *then* - # copying the new one won't change the existing memory mapped file, - # and so is safe to do. - try: - os.remove(ext_path) - except FileNotFoundError: - pass - - os.rename(dylib_path, ext_path) + # We first copy the file to the same directory, as `os.rename` + # doesn't work across file system boundaries. + temp_ext_path = ext_path + "~" + shutil.copyfile(dylib_path, temp_ext_path) + os.rename(temp_ext_path, ext_path) if sys.platform != "win32" and not debug_build: args = [] From 5c0222b24134f2883c2293e2c424b0831d1e1346 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 18:37:31 +0100 Subject: [PATCH 4/5] Update setuptools_rust/build.py Co-authored-by: Adam Reichold --- setuptools_rust/build.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setuptools_rust/build.py b/setuptools_rust/build.py index f498395a..26a363b1 100644 --- a/setuptools_rust/build.py +++ b/setuptools_rust/build.py @@ -356,7 +356,7 @@ def install_extension( # doesn't work across file system boundaries. temp_ext_path = ext_path + "~" shutil.copyfile(dylib_path, temp_ext_path) - os.rename(temp_ext_path, ext_path) + os.replace(temp_ext_path, ext_path) if sys.platform != "win32" and not debug_build: args = [] From 6fd420515ae10653865bb29e98af97dfc079d1ec Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 29 Sep 2022 19:10:22 +0100 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24deab1e..9918498a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased +### Fixed +- Fix a bug where rebuilding the library would cause any running processes using it to segfault. [#295](https://github.com/PyO3/setuptools-rust/pull/295) + ## 1.5.2 (2022-09-19) ### Fixed - Fix regression in `dylib` build artifacts not being found since 1.5.0. [#290](https://github.com/PyO3/setuptools-rust/pull/290)