From a9ca73ae99b87746044dc5b0a322b612d06733be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?JB=20Onofr=C3=A9?= Date: Sun, 13 Sep 2026 06:58:47 +0200 Subject: [PATCH 1/3] Fix race condition corrupting cfg files when updating an existing configuration FeatureConfigInstaller.updateExistingConfig() wrote directly to the target cfg file when appending to / overriding an existing configuration. Since cfg.update() also triggers fileinstall to persist the same configuration on the CM Event Dispatcher thread, two unsynchronized writers could hit the same file at once, producing torn/corrupted content. The "new file" path already avoided this by writing to a temp file and renaming it atomically into place (KARAF-7389); this applies the same pattern to the existing-file path. Fixes #2805 --- .../service/FeatureConfigInstaller.java | 9 +++- .../service/FeatureConfigInstallerTest.java | 45 ++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java index cafd76c96aa..f67d6871bfc 100644 --- a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java +++ b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java @@ -400,11 +400,16 @@ private void updateExistingConfig(TypedProperties props, boolean append, File cf } } storage.mkdirs(); + // write to a temporary file and rename it to the target file so that a concurrent writer + // (e.g. fileinstall persisting the configuration update on the CM Event Dispatcher thread) + // never observes a partially written / corrupted cfg file + File tmpCfgFile = File.createTempFile(cfgFile.getName(), ".tmp", cfgFile.getParentFile()); if (jsonFormat) { - Configurations.buildWriter().build(new FileWriter(cfgFile)).writeConfiguration(new Hashtable(properties)); + Configurations.buildWriter().build(new FileWriter(tmpCfgFile)).writeConfiguration(new Hashtable(properties)); } else { - properties.save(cfgFile); + properties.save(tmpCfgFile); } + tmpCfgFile.renameTo(cfgFile); } private boolean isInternalKey(String key) { diff --git a/features/core/src/test/java/org/apache/karaf/features/internal/service/FeatureConfigInstallerTest.java b/features/core/src/test/java/org/apache/karaf/features/internal/service/FeatureConfigInstallerTest.java index c9497fc8afd..277bd55a2f4 100644 --- a/features/core/src/test/java/org/apache/karaf/features/internal/service/FeatureConfigInstallerTest.java +++ b/features/core/src/test/java/org/apache/karaf/features/internal/service/FeatureConfigInstallerTest.java @@ -16,14 +16,21 @@ */ package org.apache.karaf.features.internal.service; +import org.apache.felix.utils.properties.TypedProperties; import org.junit.Test; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import java.io.File; +import java.io.FileWriter; +import java.io.StringReader; +import java.lang.reflect.Method; +import java.nio.file.Files; +import java.util.Objects; public class FeatureConfigInstallerTest { - + private void substEqual(final String src, final String subst) { assertEquals(FeatureConfigInstaller.substFinalName(src), subst); } @@ -51,4 +58,40 @@ public void testSubstFinalName() { substEqual("${foo}${bar}/${bar}${foo}", foo + "/" + foo); } + /** + * GH-2805: updating an existing cfg file (append or override) must write through a + * temporary file and rename it into place, so that a concurrent writer (e.g. fileinstall + * persisting the same configuration on the CM Event Dispatcher thread) can never observe + * a partially written / corrupted cfg file, and no leftover temp file remains behind. + */ + @Test + public void testUpdateExistingConfigWritesAtomically() throws Exception { + File tmpDir = Files.createTempDirectory("karaf-feature-config-installer-test").toFile(); + System.setProperty("karaf.etc", tmpDir.getAbsolutePath()); + + File cfgFile = new File(tmpDir, "my.pid.cfg"); + try (FileWriter writer = new FileWriter(cfgFile)) { + writer.write("existing.key=existing.value\n"); + } + + TypedProperties toAppend = new TypedProperties(); + toAppend.load(new StringReader("appended.key=appended.value\n")); + + FeatureConfigInstaller installer = new FeatureConfigInstaller(null, true); + + Method updateExistingConfig = FeatureConfigInstaller.class.getDeclaredMethod( + "updateExistingConfig", TypedProperties.class, boolean.class, File.class, boolean.class); + updateExistingConfig.setAccessible(true); + updateExistingConfig.invoke(installer, toAppend, true, cfgFile, false); + + TypedProperties result = new TypedProperties(); + result.load(cfgFile); + assertEquals("existing.value", result.get("existing.key")); + assertEquals("appended.value", result.get("appended.key")); + + File[] leftovers = tmpDir.listFiles((dir, name) -> name.endsWith(".tmp")); + assertTrue("no temporary file must be left behind after the atomic rename", + Objects.requireNonNull(leftovers).length == 0); + } + } From d5badd00c89559dabf920eb77340af5f0c85a2f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?JB=20Onofr=C3=A9?= Date: Sun, 13 Sep 2026 07:49:17 +0200 Subject: [PATCH 2/3] GH-2805: Use Files.move with REPLACE_EXISTING instead of File.renameTo File.renameTo() silently fails (returns false, no exception) on Windows when the destination already exists, leaving the cfg file stale and the temp file orphaned. This broke updateExistingConfig() since it is only called when the target file already exists, causing the Windows CI job to fail (AppendTest). --- .../internal/service/FeatureConfigInstaller.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java index f67d6871bfc..683d4c808ed 100644 --- a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java +++ b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java @@ -21,6 +21,8 @@ import java.net.URI; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; import java.util.*; import java.util.regex.Pattern; @@ -330,7 +332,8 @@ protected void updateStorage(ConfigId cid, TypedProperties props, boolean append } else { props.save(tmpCfgFile); } - tmpCfgFile.renameTo(cfgFile); + Files.move(tmpCfgFile.toPath(), cfgFile.toPath(), + StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); } else { updateExistingConfig(props, append, cfgFile, jsonFormat); } @@ -409,7 +412,10 @@ private void updateExistingConfig(TypedProperties props, boolean append, File cf } else { properties.save(tmpCfgFile); } - tmpCfgFile.renameTo(cfgFile); + // File.renameTo() silently fails on Windows when the destination already exists, + // so use Files.move() with REPLACE_EXISTING to get a working atomic replace on every OS + Files.move(tmpCfgFile.toPath(), cfgFile.toPath(), + StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); } private boolean isInternalKey(String key) { From 741ff3b0628ba961f3e6d5c17c3c78896b7bd963 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?JB=20Onofr=C3=A9?= Date: Sun, 13 Sep 2026 08:08:23 +0200 Subject: [PATCH 3/3] Add fallback for atomic file move Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../features/internal/service/FeatureConfigInstaller.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java index 683d4c808ed..239abc3b670 100644 --- a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java +++ b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java @@ -332,8 +332,12 @@ protected void updateStorage(ConfigId cid, TypedProperties props, boolean append } else { props.save(tmpCfgFile); } - Files.move(tmpCfgFile.toPath(), cfgFile.toPath(), - StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); +try { + Files.move(tmpCfgFile.toPath(), cfgFile.toPath(), + StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); + } catch (java.nio.file.AtomicMoveNotSupportedException e) { + Files.move(tmpCfgFile.toPath(), cfgFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + } } else { updateExistingConfig(props, append, cfgFile, jsonFormat); }