Copilot commented on code in PR #2897:
URL: https://github.com/apache/karaf/pull/2897#discussion_r3998894682
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java:
##########
@@ -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);
Review Comment:
`ATOMIC_MOVE` is optional and throws `AtomicMoveNotSupportedException` on
providers that do not implement it. This exception is only logged by the
callers, so configuration persistence is skipped and the temporary file is left
behind on those filesystems. Use the same non-atomic fallback already used in
`SimpleDownloadTask.java:92-97`.
This issue also appears on line 417 of the same file.
##########
features/core/src/test/java/org/apache/karaf/features/internal/service/FeatureConfigInstallerTest.java:
##########
@@ -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);
Review Comment:
This test does not detect the regression it names: the previous direct
`properties.save(cfgFile)` implementation also produces both expected values
and leaves zero `.tmp` files. Consequently, reverting the atomic replacement
would still pass. Add an observable concurrency/replacement check (or inject
the move operation) that fails for a direct target write, and exercise the
changed JSON path too.
##########
features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java:
##########
@@ -400,11 +403,19 @@ 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());
Review Comment:
Replacing an existing config with a newly created temporary inode discards
the target's filesystem metadata. A cfg with restrictive POSIX permissions or
ACLs can therefore inherit broader temp-file defaults after an update,
potentially exposing secrets; the previous in-place write retained those
attributes. Preserve the target's applicable permissions/ACLs on the temporary
file before moving it into place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
