Skip to content

[FELIX-6855] FileInstall prevent duplicate *.cfg files in subdirectories from corrupting live OSGi configurations #544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jefiya-MJ wants to merge 2 commits into
base: master
Choose a base branch
from
+163 −0
Open

[FELIX-6855] FileInstall prevent duplicate *.cfg files in subdirectories from corrupting live OSGi configurations #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8521a35
fix(fileinstall): skip setConfig() and deleteConfig() when file path …
Jefiya-MJ Aug 13, 2026
3a1be31
Prevent duplicate *.cfg files in subdirectories from corrupting live …
Jefiya-MJ Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment on lines 388 to +395

@paulrutter paulrutter Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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;
}
}
Comment on lines +396 to +400

@paulrutter paulrutter Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Hashtable<String, Object> old = props != null ? new Hashtable<String, Object>(new DictionaryAsMap<>(props)) : null;
if (old != null) {
old.remove( DirectoryWatcher.FILENAME );
Expand Down Expand Up @@ -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))) {

@paulrutter paulrutter Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
}
Comment on lines +454 to +456

@paulrutter paulrutter Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Util.log(context, Logger.LOG_INFO, "Deleting configuration {"
+ config.getPid()
+ "} from " + f.getAbsolutePath(), null);
Expand Down
122 changes: 122 additions & 0 deletions fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ public void testDeleteConfig() throws Exception
.andReturn(null);
EasyMock.expect(mockConfigurationAdmin.getConfiguration("pid", "?" ))
.andReturn(mockConfiguration);
EasyMock.expect(mockConfiguration.getProperties()).andReturn(null);
EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle);

ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, new FileInstall() );
Expand All @@ -227,6 +228,127 @@ public void testDeleteConfig() throws Exception
EasyMock.verify(mockConfiguration, mockConfigurationAdmin, mockBundleContext);
}

// verify that deleteConfig() skips deletion when the file being deleted
// is a duplicate (different path) from the registered live configuration.
public void testDeleteConfigSkipsWhenPathDiffersFromRegisteredSource() throws Exception
{
File canonicalFile = new File( "src/test/resources/watched/firstcfg.cfg" );
File backupFile = new File( "src/test/resources/watched/backup/firstcfg.cfg" );

Hashtable<String, Object> props = new Hashtable<>();
props.put( DirectoryWatcher.FILENAME, canonicalFile.getAbsoluteFile().toURI().toString() );

EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes();
EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject()))
.andReturn(null)
.anyTimes();
EasyMock.expect(mockConfigurationAdmin.listConfigurations((String) EasyMock.anyObject()))
.andReturn(null);
EasyMock.expect(mockConfigurationAdmin.getConfiguration("firstcfg", "?"))
.andReturn(mockConfiguration);
EasyMock.expect(mockConfiguration.getProperties()).andReturn(props);
EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle);

ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, new FileInstall() );

assertFalse( ci.deleteConfig( backupFile ) );

EasyMock.verify(mockConfiguration, mockConfigurationAdmin, mockBundleContext);
}

// verify that deleteConfig() proceeds normally when the file being deleted
// is the registered source of the live configuration.
public void testDeleteConfigProceedsWhenPathMatchesRegisteredSource() throws Exception
{
File canonicalFile = new File( "src/test/resources/watched/firstcfg.cfg" );

Hashtable<String, Object> props = new Hashtable<>();
props.put( DirectoryWatcher.FILENAME, canonicalFile.getAbsoluteFile().toURI().toString() );

mockConfiguration.delete();
EasyMock.expect(mockConfiguration.getPid()).andReturn("firstcfg");
EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes();
EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject()))
.andReturn(null)
.anyTimes();
EasyMock.expect(mockConfigurationAdmin.listConfigurations((String) EasyMock.anyObject()))
.andReturn(null);
EasyMock.expect(mockConfigurationAdmin.getConfiguration("firstcfg", "?"))
.andReturn(mockConfiguration);
EasyMock.expect(mockConfiguration.getProperties()).andReturn(props);
EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle);

ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, new FileInstall() );

assertTrue( ci.deleteConfig( canonicalFile ) );

EasyMock.verify(mockConfiguration, mockConfigurationAdmin, mockBundleContext);
}

// Verify that setConfig() skips the update when the file being set is a duplicate
// (different path) from the registered live configuration.
public void testSetConfigSkipsWhenPathDiffersFromRegisteredSource() throws Exception
{
File canonicalFile = new File( "src/test/resources/watched/firstcfg.cfg" );
File backupFile = new File( "src/test/resources/watched/backup/firstcfg.cfg" );

Hashtable<String, Object> props = new Hashtable<>();
props.put( DirectoryWatcher.FILENAME, canonicalFile.getAbsoluteFile().toURI().toString() );

EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes();
EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject()))
.andReturn(null)
.anyTimes();
EasyMock.expect(mockConfigurationAdmin.listConfigurations((String) EasyMock.anyObject()))
.andReturn(null);
EasyMock.expect(mockConfigurationAdmin.getConfiguration("firstcfg", "?"))
.andReturn(mockConfiguration);
EasyMock.expect(mockConfiguration.getAttributes()).andReturn(Collections.emptySet()).anyTimes();
EasyMock.expect(mockConfiguration.getProperties()).andReturn(props);
EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle);

ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, new FileInstall() );

assertFalse( ci.setConfig( backupFile ) );

EasyMock.verify(mockConfiguration, mockConfigurationAdmin, mockBundleContext);
}

// Verify that setConfig() proceeds normally when the file being set is the
// registered source of the live configuration.
public void testSetConfigProceedsWhenPathMatchesRegisteredSource() throws Exception
{
File canonicalFile = new File( "src/test/resources/watched/firstcfg.cfg" );

Hashtable<String, Object> props = new Hashtable<>();
props.put( DirectoryWatcher.FILENAME, canonicalFile.getAbsoluteFile().toURI().toString() );

EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes();
EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject()))
.andReturn(null)
.anyTimes();
EasyMock.expect(mockConfigurationAdmin.listConfigurations((String) EasyMock.anyObject()))
.andReturn(null);
EasyMock.expect(mockConfigurationAdmin.getConfiguration("firstcfg", "?"))
.andReturn(mockConfiguration);
EasyMock.expect(mockConfiguration.getProperties()).andReturn(props).anyTimes();
EasyMock.expect(mockConfiguration.getAttributes()).andReturn(Collections.emptySet()).times(2);
EasyMock.expect(mockConfiguration.getPid()).andReturn("firstcfg");
EasyMock.expect(mockConfiguration.updateIfDifferent((Dictionary<String, Object>) EasyMock.anyObject()))
.andReturn(true);
EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle);

ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, new FileInstall() );

assertTrue( ci.setConfig( canonicalFile ) );

EasyMock.verify(mockConfiguration, mockConfigurationAdmin, mockBundleContext);
}

public void testCreateConfigAndObserveCMDeleted() throws Exception
{
File file = File.createTempFile("test", ".config");
Expand Down
19 changes: 19 additions & 0 deletions fileinstall/src/test/resources/watched/backup/firstcfg.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#
testkey=testvalue