This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch karaf-4.4.x
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/karaf-4.4.x by this push:
new 625c96c736 [karaf-4.4.x] Fix race condition corrupting cfg files when
updating an existing configuration (#2900)
625c96c736 is described below
commit 625c96c736dc49b548868ec5506d018f3315356e
Author: JB Onofré <[email protected]>
AuthorDate: Mon Sep 14 15:20:41 2026 +0200
[karaf-4.4.x] Fix race condition corrupting cfg files when updating an
existing configuration (#2900)
* 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
(cherry picked from commit a9ca73ae99b87746044dc5b0a322b612d06733be)
* 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).
(cherry picked from commit d5badd00c89559dabf920eb77340af5f0c85a2f3)
* Add fallback for atomic file move
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
(cherry picked from commit 741ff3b0628ba961f3e6d5c17c3c78896b7bd963)
---------
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 7d530ae4e6..e903e2ce53 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);
+ }
+
}
