Conversation
…does not match registered source
…OSGi configurations
Jefiya-MJ
changed the title
|
Can someone take a look into this PR? TIA |
paulrutter
left a comment
paulrutter
left a comment
There was a problem hiding this comment.
Review of the duplicate-PID guard. The direction looks right, but the placement of the setConfig() check leaks the READ_ONLY attribute, and the skip is permanent (never retried), which can lose a configuration outright on a legitimate file move. Details inline.
| 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) { |
There was a problem hiding this 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:
| 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) { | |
| 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); |
| String registeredFileName = (String) props.get(DirectoryWatcher.FILENAME); | ||
| if (registeredFileName != null && !registeredFileName.equals(toConfigKey(f))) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this 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):
- Cycle N:
/watched/sub/app.cfgappears.setConfig()sees PIDappowned by/watched/etc/app.cfg, returnsfalse. DirectoryWatcher still recordssub/app.cfg+ checksum as installed. - Cycle N+1:
/watched/etc/app.cfgis deleted.deleteConfig()matches the registered filename and deletes the configuration. sub/app.cfgis still on disk, unchanged, and is already incurrentManagedArtifacts— 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.
| Dictionary<String, Object> props = config.getProperties(); | ||
| if (props != null) { | ||
| String registeredFileName = (String) props.get(DirectoryWatcher.FILENAME); | ||
| if (registeredFileName != null && !registeredFileName.equals(toConfigKey(f))) { |
There was a problem hiding this 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:
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.)
| return false; | ||
| } | ||
| } |
There was a problem hiding this 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.
Proposal: wire
|
| JDK | Result |
|---|---|
| 21 | Tests run: 32, Failures: 0, Errors: 0 — BUILD SUCCESS |
| 25 | Tests run: 32, Failures: 0, Errors: 0 — BUILD SUCCESS |
JDK 25 stands in as an upper bound for the matrix's 23. I could not verify JDK 17 locally (not packaged for my distro) — worth a glance at the first CI run, though source/target 8 on 17 is the least risky of the three.
Note for anyone reproducing on Windows: 5 further tests in DirectoryWatcherTest/ConfigInstallerTest fail there because they build expected URIs by string concatenation ("file:" + absolutePath, which yields file:C:\... instead of file:/C:/...). Those are Windows-only and I left them untouched, since CI is Linux.
Diff
diff --git a/.github/workflows/maven-ci.yml b/.github/workflows/maven-ci.yml
index 3658e01105..cf36e0bc76 100644
--- a/.github/workflows/maven-ci.yml
+++ b/.github/workflows/maven-ci.yml
@@ -11,6 +11,7 @@ on:
- 'log/**'
- 'webconsole/**'
- 'framework/**'
+ - 'fileinstall/**'
pull_request:
branches: [ "master" ]
paths:
@@ -22,6 +23,7 @@ on:
- 'log/**'
- 'framework/**'
- 'gogo/**'
+ - 'fileinstall/**'
permissions: {}
@@ -65,6 +67,8 @@ jobs:
- 'framework/**'
gogo:
- 'gogo/**'
+ fileinstall:
+ - 'fileinstall/**'
- name: Felix SCR
if: steps.changes.outputs.scr == 'true'
@@ -93,6 +97,13 @@ jobs:
- name: Felix Gogo Shell
if: steps.changes.outputs.gogo == 'true'
run: mvn -B -V -Dstyle.color=always --file gogo/pom.xml clean verify
+ # fileinstall still inherits felix-parent 6, which binds ianal-maven-plugin
+ # 1.0-alpha-1. That plugin fails on JDK 17+ ("module java.base does not opens
+ # java.io"), so the 'verify' phase cannot run here. Stick to 'test' until the
+ # module is moved to a newer parent.
+ - name: Felix File Install
+ if: steps.changes.outputs.fileinstall == 'true'
+ run: mvn -B -V -Dstyle.color=always --file fileinstall/pom.xml clean test
- name: Upload Test Results
if: always()
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
diff --git a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
index 6769c4d3d5..49d02467fd 100644
--- a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
+++ b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
@@ -74,6 +74,8 @@ public class DirectoryWatcherTest extends TestCase
.andStubReturn(null);
EasyMock.expect(mockBundleContext.getServiceReference(LogService.class.getName()))
.andStubReturn(null);
+ EasyMock.expect(mockBundleContext.getServiceReference(LogService.class))
+ .andStubReturn(null);
EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
EasyMock.expect(mockBundleContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION)).andReturn(mockSysBundle).anyTimes();
EasyMock.expect(mockSysBundle.getState()).andReturn(Bundle.ACTIVE).anyTimes();
@@ -237,39 +239,27 @@ public class DirectoryWatcherTest extends TestCase
public void testInvalidTempDir() throws Exception
{
- String oldTmpDir = System.getProperty("java.io.tmpdir");
-
- try
+ // Point felix.fileinstall.tmpdir at a path whose parent is a regular file, so that
+ // creating the directory is bound to fail for any user running the build (root
+ // included). Overriding java.io.tmpdir would not work here: Files.createTempDirectory
+ // caches that property on first use, so a later System.setProperty has no effect.
+ File blocker = new File( "target/not-a-directory" );
+ blocker.getParentFile().mkdirs();
+ blocker.delete();
+ assertTrue( "Unable to create test fixture file " + blocker, blocker.createNewFile() );
+
+ props.put( DirectoryWatcher.TMPDIR, new File( blocker, "tmp" ).getAbsolutePath() );
+
+ EasyMock.replay(mockBundleContext, mockBundle, mockSysBundle, mockStartLevel);
+
+ try
{
- File parent = new File("target/tmp");
- parent.mkdirs();
- parent.setWritable(false, false);
- File tmp = new File(parent, "tmp");
- System.setProperty("java.io.tmpdir", tmp.toString());
-
- mockBundleContext.addBundleListener((BundleListener) org.easymock.EasyMock.anyObject());
- EasyMock.expect(mockBundleContext.createFilter((String) EasyMock.anyObject()))
- .andReturn(null);
-
- BundleRevision mockBundleRevision = EasyMock.createNiceMock(BundleRevision.class);
- EasyMock.expect(mockBundle.adapt(BundleRevision.class)).andReturn(mockBundleRevision);
- EasyMock.expect(mockBundleRevision.getTypes())
- .andReturn(BundleRevision.TYPE_FRAGMENT);
- EasyMock.replay(mockBundleContext, mockBundle, mockBundleRevision, mockSysBundle, mockStartLevel);
-
- try
- {
- dw = new DirectoryWatcher( new FileInstall(), props, mockBundleContext );
- fail("Expected an IllegalStateException");
- }
- catch (IllegalStateException e)
- {
- // expected
- }
+ dw = new DirectoryWatcher( new FileInstall(), props, mockBundleContext );
+ fail("Expected a RuntimeException for a temp directory that cannot be created");
}
- finally
+ catch (RuntimeException e)
{
- System.setProperty("java.io.tmpdir", oldTmpDir);
+ // expected
}
}
2 participants
Problem
When
felix.fileinstall.subdir.mode = recurseis active, FileInstall scans all subdirectories under the watched root and registers every matching file as an OSGi Configuration object.Both
setConfig()anddeleteConfig()derive the ConfigurationAdmin PID from the filename alone viaparsePid(f.getName()), discarding the directory path entirely. This means any two files with the same name —regardless of where they sit in the directory tree — resolve to the same PID.
Consequences:
If a file with the same name as a live configuration is copied into a subdirectory,
setConfig()overwrites the live configuration'sfelix.fileinstall.filenameproperty with the duplicate file's URI, silently stealing ownership.When the duplicate file is later deleted,
deleteConfig()resolves the same PID and callsconfig.delete()on the live Configuration object — removing it from ConfigurationAdmin even though the original physical file was never touched. Dependent OSGi services stop immediately and cannot restart.Fix
Before acting on a resolved Configuration object, both
setConfig()anddeleteConfig()now compare the absolute URI of the file under operation against thefelix.fileinstall.filenameproperty already stored in that Configuration.Reproduction Scenario
felix.fileinstall.subdir.mode = recurse(default)./watched/etc/app.cfgas PIDapp./watched/etc/app.cfgto/watched/backup/etc/app.cfg./watched/backup/etc/app.cfg.deleteConfig()resolves PIDappand removes the live configuration from ConfigurationAdmin. The service stops.deleteConfig()detects the URI mismatch and returnsfalsewithout touching ConfigurationAdmin.