summaryrefslogtreecommitdiff
path: root/lib
diff options
context:
space:
mode:
authorHarmen Stoppels <harmenstoppels@gmail.com>2022-11-08 20:05:05 +0100
committerGitHub <noreply@github.com>2022-11-08 11:05:05 -0800
commit4d28a6466188ea9fe3b55a4c3d7690dd66e0dc8f (patch)
treef1e1cf3f466e5ee21f109c5583ff481c8aace294 /lib
parent4a5e68816bdb83d7b614e586042a1927b7267576 (diff)
downloadspack-4d28a6466188ea9fe3b55a4c3d7690dd66e0dc8f.tar.gz
spack-4d28a6466188ea9fe3b55a4c3d7690dd66e0dc8f.tar.bz2
spack-4d28a6466188ea9fe3b55a4c3d7690dd66e0dc8f.tar.xz
spack-4d28a6466188ea9fe3b55a4c3d7690dd66e0dc8f.zip
fix racy sbang (#33549)
Spack currently creates a temporary sbang that is moved "atomically" in place, but this temporary causes races when multiple processes start installing sbang. Let's just stick to an idempotent approach. Notice that we only re-install sbang if Spack updates it (since we do file compare), and sbang was only touched 18 times in the past 6 years, whereas we hit the sbang tempfile issue frequently with parallel install on a fresh spack instance in CI. Also fixes a bug where permissions weren't updated if config changed but the latest version of the sbang file was already installed.
Diffstat (limited to 'lib')
-rw-r--r--lib/spack/llnl/util/filesystem.py45
-rw-r--r--lib/spack/spack/config.py6
-rw-r--r--lib/spack/spack/hooks/sbang.py57
3 files changed, 65 insertions, 43 deletions
diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index aece52f843..d122280e99 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -1000,16 +1000,45 @@ def hash_directory(directory, ignore=[]):
return md5_hash.hexdigest()
+def _try_unlink(path):
+ try:
+ os.unlink(path)
+ except (IOError, OSError):
+ # But if that fails, that's OK.
+ pass
+
+
@contextmanager
@system_path_filter
-def write_tmp_and_move(filename):
- """Write to a temporary file, then move into place."""
- dirname = os.path.dirname(filename)
- basename = os.path.basename(filename)
- tmp = os.path.join(dirname, ".%s.tmp" % basename)
- with open(tmp, "w") as f:
- yield f
- shutil.move(tmp, filename)
+def write_tmp_and_move(path, mode="w"):
+ """Write to a temporary file in the same directory, then move into place."""
+ # Rely on NamedTemporaryFile to give a unique file without races
+ # in the directory of the target file.
+ file = tempfile.NamedTemporaryFile(
+ prefix="." + os.path.basename(path),
+ suffix=".tmp",
+ dir=os.path.dirname(path),
+ mode=mode,
+ delete=False, # we delete it ourselves
+ )
+ tmp_path = file.name
+
+ try:
+ yield file
+ except BaseException:
+ # On any failure, try to remove the temporary file.
+ _try_unlink(tmp_path)
+ raise
+ finally:
+ # Always close the file decriptor
+ file.close()
+
+ # Atomically move into existence.
+ try:
+ os.rename(tmp_path, path)
+ except (IOError, OSError):
+ _try_unlink(tmp_path)
+ raise
@contextmanager
diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index e9a3b1d956..f35310986e 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -45,7 +45,7 @@ from six import iteritems
import llnl.util.lang
import llnl.util.tty as tty
-from llnl.util.filesystem import mkdirp, rename
+from llnl.util.filesystem import mkdirp, write_tmp_and_move
import spack.compilers
import spack.paths
@@ -287,10 +287,8 @@ class SingleFileScope(ConfigScope):
parent = os.path.dirname(self.path)
mkdirp(parent)
- tmp = os.path.join(parent, ".%s.tmp" % os.path.basename(self.path))
- with open(tmp, "w") as f:
+ with write_tmp_and_move(self.path) as f:
syaml.dump_config(data_to_write, stream=f, default_flow_style=False)
- rename(tmp, self.path)
except (yaml.YAMLError, IOError) as e:
raise ConfigFileError("Error writing to config file: '%s'" % str(e))
diff --git a/lib/spack/spack/hooks/sbang.py b/lib/spack/spack/hooks/sbang.py
index 5d2de2b865..e22c2929c2 100644
--- a/lib/spack/spack/hooks/sbang.py
+++ b/lib/spack/spack/hooks/sbang.py
@@ -186,44 +186,39 @@ def install_sbang():
``sbang`` here ensures that users can access the script and that
``sbang`` itself is in a short path.
"""
- # copy in a new version of sbang if it differs from what's in spack
sbang_path = sbang_install_path()
- if os.path.exists(sbang_path) and filecmp.cmp(spack.paths.sbang_script, sbang_path):
- return
- # make $install_tree/bin
- sbang_bin_dir = os.path.dirname(sbang_path)
- fs.mkdirp(sbang_bin_dir)
+ all = spack.spec.Spec("all")
+ group_name = spack.package_prefs.get_package_group(all)
+ config_mode = spack.package_prefs.get_package_dir_permissions(all)
+ group_id = grp.getgrnam(group_name).gr_gid if group_name else None
- # get permissions for bin dir from configuration files
- group_name = spack.package_prefs.get_package_group(spack.spec.Spec("all"))
- config_mode = spack.package_prefs.get_package_dir_permissions(spack.spec.Spec("all"))
+ # First setup the bin dir correctly.
+ sbang_bin_dir = os.path.dirname(sbang_path)
+ if not os.path.isdir(sbang_bin_dir):
+ fs.mkdirp(sbang_bin_dir)
- if group_name:
- os.chmod(sbang_bin_dir, config_mode) # Use package directory permissions
+ # Set group and ownership like we do on package directories
+ if group_id:
+ os.chown(sbang_bin_dir, os.stat(sbang_bin_dir).st_uid, group_id)
+ os.chmod(sbang_bin_dir, config_mode)
else:
fs.set_install_permissions(sbang_bin_dir)
- # set group on sbang_bin_dir if not already set (only if set in configuration)
- # TODO: after we drop python2 support, use shutil.chown to avoid gid lookups that
- # can fail for remote groups
- if group_name and os.stat(sbang_bin_dir).st_gid != grp.getgrnam(group_name).gr_gid:
- os.chown(sbang_bin_dir, os.stat(sbang_bin_dir).st_uid, grp.getgrnam(group_name).gr_gid)
-
- # copy over the fresh copy of `sbang`
- sbang_tmp_path = os.path.join(
- os.path.dirname(sbang_path),
- ".%s.tmp" % os.path.basename(sbang_path),
- )
- shutil.copy(spack.paths.sbang_script, sbang_tmp_path)
-
- # set permissions on `sbang` (including group if set in configuration)
- os.chmod(sbang_tmp_path, config_mode)
- if group_name:
- os.chown(sbang_tmp_path, os.stat(sbang_tmp_path).st_uid, grp.getgrnam(group_name).gr_gid)
-
- # Finally, move the new `sbang` into place atomically
- os.rename(sbang_tmp_path, sbang_path)
+ # Then check if we need to install sbang itself.
+ try:
+ already_installed = filecmp.cmp(spack.paths.sbang_script, sbang_path)
+ except (IOError, OSError):
+ already_installed = False
+
+ if not already_installed:
+ with fs.write_tmp_and_move(sbang_path) as f:
+ shutil.copy(spack.paths.sbang_script, f.name)
+
+ # Set permissions on `sbang` (including group if set in configuration)
+ os.chmod(sbang_path, config_mode)
+ if group_id:
+ os.chown(sbang_path, os.stat(sbang_path).st_uid, group_id)
def post_install(spec):