This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/main by this push:
new 772fcec2d7 Fix race condition corrupting cfg files when updating an
existing configuration (#2897)
772fcec2d7 is described below
commit 772fcec2d72bcad44328435c804a8e47f45c5f28
Author: JB Onofré <[email protected]>
AuthorDate: Mon Sep 14 15:20:03 2026 +0200
Fix race condition corrupting cfg files when updating an existing
configuration (#2897)
* 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
* 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).
* Add fallback for atomic file move
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
---------
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
---
.../internal/service/FeatureConfigInstaller.java | 21 ++++++++--
.../service/FeatureConfigInstallerTest.java | 45 +++++++++++++++++++++-
2 files changed, 62 insertions(+), 4 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 cafd76c96a..239abc3b67 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.MalformedURLException;
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,12 @@ public class FeatureConfigInstaller {
} else {
props.save(tmpCfgFile);
}
- tmpCfgFile.renameTo(cfgFile);
+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);
}
@@ -400,11 +407,19 @@ public class FeatureConfigInstaller {
}
}
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);
}
+ // 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) {
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 c9497fc8af..277bd55a2f 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 class FeatureConfigInstallerTest {
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);
+ }
+
}
