paulrutter commented on code in PR #544:
URL: https://github.com/apache/felix-dev/pull/544#discussion_r4013209468
##########
fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java:
##########
@@ -433,6 +444,17 @@ boolean deleteConfig(File f) throws Exception
{
String pid[] = parsePid(f.getName());
Configuration config = getConfiguration(toConfigKey(f), pid[0],
pid[1]);
+
+ // Only delete if this file is the registered source of the
configuration.
+ // If the registered felix.fileinstall.filename does not match the
file being deleted, skip deletion to protect the live config.
+ Dictionary<String, Object> props = config.getProperties();
+ if (props != null) {
+ String registeredFileName = (String)
props.get(DirectoryWatcher.FILENAME);
+ if (registeredFileName != null &&
!registeredFileName.equals(toConfigKey(f))) {
Review Comment:
**Comparing the two URIs as raw strings instead of as file identities makes
the guard fire on paths that denote the same file.**
`toConfigKey()` is `f.getAbsoluteFile().toURI().toString()`, and
`getAbsoluteFile()` does not normalize `.`/`..` segments or (on Windows)
drive-letter case.
Scenario: an admin initially sets `felix.fileinstall.dir=./etc`, so the
configuration is stored with
`felix.fileinstall.filename=file:/opt/app/./etc/app.cfg`. Later they tidy the
property to `felix.fileinstall.dir=/opt/app/etc`. After restart,
`toConfigKey()` yields `file:/opt/app/etc/app.cfg`, which is `!equals` the
stored value even though it is the very same file. From then on `setConfig()`
silently returns `false` — edits to `app.cfg` never reach ConfigAdmin — and
`deleteConfig()` refuses to remove the configuration when the file is deleted.
Windows `c:/watched` vs `C:/watched` is the same failure class.
Comparing resolved files rather than strings avoids this:
```java
String registeredFileName = (String) props.get(DirectoryWatcher.FILENAME);
if (registeredFileName != null
&&
!fromConfigKey(registeredFileName).getAbsoluteFile().equals(f.getAbsoluteFile()))
{
return false;
}
```
(`File.equals` applies the platform's case rules; `getCanonicalFile()` would
additionally resolve symlinks, at the cost of an I/O call.)
##########
fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java:
##########
@@ -388,6 +388,17 @@ boolean setConfig(final File f) throws Exception
clearReadOnlyIfWritable(pid, config, f);
Dictionary<String, Object> props = config.getProperties();
+
+ // Only update if this file is the registered source of the
configuration,
+ // or if no source has been registered yet (new configuration).
+ // Skips duplicate files that share the same PID but originate from a
different path.
+ if (props != null) {
Review Comment:
**The early return sits after `clearReadOnlyIfWritable()` but before the
`try`/`finally` that calls `setReadOnlyInNotWritable()`, so it leaks the
READ_ONLY attribute of the *live* configuration.**
Scenario: `/watched/etc/app.cfg` is read-only, so the live configuration for
PID `app` carries the READ_ONLY attribute. A *writable* duplicate
`/watched/backup/app.cfg` is then dropped into a subdirectory.
`clearReadOnlyIfWritable(pid, config, f)` (line 388) sees
`Util.canWrite(duplicate) == true` and strips READ_ONLY from the live config.
The new check then returns `false`, so the `finally {
setReadOnlyInNotWritable(...) }` block is never reached and the attribute is
never restored. The live configuration is left permanently writable — exactly
the kind of cross-file interference this PR sets out to prevent.
The check needs to run *before* `clearReadOnlyIfWritable`:
```suggestion
Dictionary<String, Object> props = config.getProperties();
// Only update if this file is the registered source of the
configuration,
// or if no source has been registered yet (new configuration).
// Skips duplicate files that share the same PID but originate from
a different path.
if (props != null) {
String registeredFileName = (String)
props.get(DirectoryWatcher.FILENAME);
if (registeredFileName != null &&
!registeredFileName.equals(toConfigKey(f))) {
Util.log(context, Logger.LOG_WARNING, "Skipping
configuration update for "
+ f.getAbsolutePath() + ": PID {" + config.getPid()
+ "} is already owned by " + registeredFileName,
null);
return false;
}
}
clearReadOnlyIfWritable(pid, config, f);
```
##########
fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java:
##########
@@ -388,6 +388,17 @@ boolean setConfig(final File f) throws Exception
clearReadOnlyIfWritable(pid, config, f);
Dictionary<String, Object> props = config.getProperties();
+
+ // Only update if this file is the registered source of the
configuration,
+ // or if no source has been registered yet (new configuration).
+ // Skips duplicate files that share the same PID but originate from a
different path.
+ if (props != null) {
+ String registeredFileName = (String)
props.get(DirectoryWatcher.FILENAME);
+ if (registeredFileName != null &&
!registeredFileName.equals(toConfigKey(f))) {
+ return false;
+ }
+ }
Review Comment:
**The skip is permanent and never retried, so a legitimate file move
detected across two scan cycles loses the configuration for good.**
`DirectoryWatcher.install(Artifact)` discards the return value of
`ArtifactInstaller.install()` and then unconditionally calls `setArtifact(path,
artifact)`, recording the file and its checksum as successfully installed
(`DirectoryWatcher.java:940-972`). So once `setConfig()` returns `false` here,
the file will never be re-offered until its bytes change.
Scenario (`cp` then `rm`, one scan cycle apart — 2s default poll, so easy to
hit):
1. Cycle N: `/watched/sub/app.cfg` appears. `setConfig()` sees PID `app`
owned by `/watched/etc/app.cfg`, returns `false`. DirectoryWatcher still
records `sub/app.cfg` + checksum as installed.
2. Cycle N+1: `/watched/etc/app.cfg` is deleted. `deleteConfig()` matches
the registered filename and deletes the configuration.
3. `sub/app.cfg` is still on disk, unchanged, and is already in
`currentManagedArtifacts` — it is never re-processed. The configuration is gone
permanently and no restart-free recovery exists.
Before this patch the config survived step 2 (re-created from the surviving
file on the next change). Same root cause produces a permanently orphaned
`.cfg` whenever a duplicate happens to be scanned *first* on startup (directory
iteration order decides which file wins, and the loser is locked out silently).
Consider recording rejected paths and re-evaluating them when the owning
configuration disappears (`configurationEvent` already handles `CM_DELETED`),
or routing the rejection through `DirectoryWatcher.processingFailures` so it
gets retried.
##########
fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java:
##########
@@ -433,6 +444,17 @@ boolean deleteConfig(File f) throws Exception
{
String pid[] = parsePid(f.getName());
Configuration config = getConfiguration(toConfigKey(f), pid[0],
pid[1]);
+
+ // Only delete if this file is the registered source of the
configuration.
+ // If the registered felix.fileinstall.filename does not match the
file being deleted, skip deletion to protect the live config.
+ Dictionary<String, Object> props = config.getProperties();
+ if (props != null) {
+ String registeredFileName = (String)
props.get(DirectoryWatcher.FILENAME);
+ if (registeredFileName != null &&
!registeredFileName.equals(toConfigKey(f))) {
+ return false;
+ }
+ }
Review Comment:
**Silent skip: this is the only exit from `deleteConfig()` that logs
nothing.**
Every other outcome in `setConfig()`/`deleteConfig()` emits a
`Util.log(...)` line ("Creating/Updating/Deleting configuration ..."). When
this guard triggers, an admin sees a `.cfg` file being created or deleted in a
watched directory with no effect on ConfigAdmin and nothing at all in the log
to explain why — which makes the duplicate-PID situation the patch detects
effectively undiagnosable in production. Please log at WARNING with the file
path, the PID, and the registered owner.
Also, the javadoc above still reads `@return <code>true</code>`, which is no
longer accurate now that `false` is reachable.
--
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]
