From 24cce8b4ee40bc83077fa4b76eccf9e407c1ca2b Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 15 Sep 2026 16:30:37 +0200 Subject: [PATCH 1/2] FELIX-6812 Follow-up: escape the pid in the service.pid filter FELIX-6812 replaced getConfiguration(pid, "?") with a listConfigurations call, and it builds the filter by concatenating the pid. A pid is not a filter-safe value, and the class already escapes one in findExistingConfiguration. A pid holding an asterisk builds a substring filter. listConfigurations then returns configurations the event was not about, and ConfigInstaller writes the first of them back to that other configuration's file. A pid holding a parenthesis throws InvalidSyntaxException instead, which the catch reports as a failure to save, so the configuration is never written back. escapeFilterValue left the backslash alone, although the OSGi filter grammar reserves it. A pid holding a backslash therefore stayed ambiguous after the escaping. The backslash is escaped first, or the escapes added after it would be escaped a second time. The method now matches literals rather than patterns. It compiled four patterns per call, and FELIX-6812 moved it from findExistingConfiguration, called once per file install, to doConfigurationEvent, called for every configuration event. Two tests raise CM_UPDATED for a pid holding an asterisk and for a pid holding a backslash, and each one fails when its escape is removed. --- .../fileinstall/internal/ConfigInstaller.java | 15 +++++--- .../internal/ConfigInstallerTest.java | 34 +++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java index 020ee0bb52..ca600596e5 100644 --- a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java +++ b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java @@ -237,7 +237,7 @@ public void doConfigurationEvent(ConfigurationEvent configurationEvent) { try { - Configuration[] configurations = getConfigurationAdmin().listConfigurations("(service.pid=" + configurationEvent.getPid() + ")"); + Configuration[] configurations = getConfigurationAdmin().listConfigurations("(service.pid=" + escapeFilterValue(configurationEvent.getPid()) + ")"); if (null == configurations) { return; } @@ -664,10 +664,15 @@ void update0(String[] pid, final Configuration config, final Hashtable answer() throws Throwable { assertFalse("Configuration file should be deleted", file.isFile()); } + public void testTheConfigurationEventFilterEscapesAnAsteriskInThePid() throws Exception + { + assertTheEventFilterFor("my*pid", "(service.pid=my\\*pid)"); + } + + public void testTheConfigurationEventFilterEscapesABackslashInThePid() throws Exception + { + assertTheEventFilterFor("my\\pid", "(service.pid=my\\\\pid)"); + } + + /** + * Raise CM_UPDATED for the given pid, and assert the filter the handler builds from it. + * An unescaped pid builds a filter that matches other configurations, so ConfigInstaller + * writes one of them back to the wrong file. EasyMock fails the call when the filter differs. + */ + private void assertTheEventFilterFor(String pid, String expectedFilter) throws Exception + { + 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(expectedFilter)) + .andReturn(null); + + ServiceReference sr = EasyMock.createMock(ServiceReference.class); + EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle, sr); + + ConfigInstaller ci = new ConfigInstaller(mockBundleContext, mockConfigurationAdmin, new FileInstall()); + ci.doConfigurationEvent(new ConfigurationEvent(sr, ConfigurationEvent.CM_UPDATED, null, pid)); + + EasyMock.verify(mockConfigurationAdmin); + } + public void testUseExistingConfigAndObserveCMDeleted() throws Exception { String pid = "test"; From 7c7f7f67bedae0afed80114abe2bcf201416daff Mon Sep 17 00:00:00 2001 From: Kevan Date: Thu, 17 Sep 2026 11:21:24 +0200 Subject: [PATCH 2/2] FELIX-6812 Follow-up: the two tests the change left green and empty FELIX-6812 moved the CM_UPDATED path from getConfiguration to listConfigurations, and two tests kept the expectations of the older call. Neither test calls EasyMock.verify, so an expectation nothing consumes fails nothing. testUseExistingConfigAndObserveCMDeleted expects listConfigurations for any argument and answers null. The handler then returns before it reads anything, so the test asserts nothing at all while it passes. It now expects the filter the handler builds and answers with a configuration. testUseExistingConfigWithFileinstallFilenameAndObserveCMDeleted still expects getConfiguration and update. The CM_UPDATED path writes the file and calls no method on the configuration, and getConfiguration is reachable from setConfig only, which this test never calls. Both tests verify their mocks now, which is what makes an expectation load-bearing. The dead expectations go, and the Capture that fed update goes with them. Adding the verify call before removing the expectations fails on Configuration.update, which is the measurement behind this commit. --- .../internal/ConfigInstallerTest.java | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java index 7da44865e7..3e8c011ebb 100644 --- a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java +++ b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java @@ -316,8 +316,6 @@ public void testUseExistingConfigAndObserveCMDeleted() throws Exception { String pid = "test"; - Capture> props = new Capture<>(); - EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes(); EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes(); EasyMock.expect(mockConfiguration.getProperties()) @@ -325,14 +323,10 @@ public void testUseExistingConfigAndObserveCMDeleted() throws Exception EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())) .andReturn(null) .anyTimes(); - EasyMock.expect(mockConfigurationAdmin.listConfigurations((String) EasyMock.anyObject())) - .andReturn(null); - EasyMock.expect(mockConfigurationAdmin.getConfiguration(pid, "?")) - .andReturn(mockConfiguration); + EasyMock.expect(mockConfigurationAdmin.listConfigurations("(service.pid=" + pid + ")")) + .andReturn(new Configuration[] { mockConfiguration }); ServiceReference sr = EasyMock.createMock(ServiceReference.class); - mockConfiguration.update(EasyMock.capture(props)); - EasyMock.expectLastCall(); EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle, sr); @@ -340,6 +334,14 @@ public void testUseExistingConfigAndObserveCMDeleted() throws Exception ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_UPDATED, null, pid ) ); ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_DELETED, null, pid ) ); + + // ConfigInstaller writes no file for a configuration that records no file name, and it + // deletes no file either. The name of this test promises an existing configuration and a + // deletion, and the test covers neither, because getProperties returns null and both + // handlers leave at their first check. The name is the one this test has always had. + // Without the call below the test passes on an early return, and an expectation that + // describes nothing stays unused. + EasyMock.verify(mockConfiguration, mockConfigurationAdmin); } public void testUseExistingConfigWithFileinstallFilenameAndObserveCMDeleted() throws Exception @@ -350,7 +352,6 @@ public void testUseExistingConfigWithFileinstallFilenameAndObserveCMDeleted() th } String pid = "test"; - Capture> propsCapture = new Capture<>(); Dictionary props = new Hashtable<>(); props.put(DirectoryWatcher.FILENAME, file.toURI().toString()); @@ -363,14 +364,10 @@ public void testUseExistingConfigWithFileinstallFilenameAndObserveCMDeleted() th .anyTimes(); EasyMock.expect(mockConfigurationAdmin.listConfigurations("(service.pid=" + pid + ")")) .andReturn(new Configuration[] { mockConfiguration }); - EasyMock.expect(mockConfigurationAdmin.getConfiguration(pid, "?")) - .andReturn(mockConfiguration); EasyMock.expect(mockConfiguration.getPid()) .andReturn(pid); ServiceReference sr = EasyMock.createMock(ServiceReference.class); - mockConfiguration.update(EasyMock.capture(propsCapture)); - EasyMock.expectLastCall(); EasyMock.replay(mockConfiguration, mockConfigurationAdmin, mockBundleContext, mockBundle, sr); @@ -380,6 +377,10 @@ public void testUseExistingConfigWithFileinstallFilenameAndObserveCMDeleted() th ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_DELETED, null, pid ) ); assertFalse("Configuration file should be deleted", file.isFile()); + + // The CM_UPDATED path writes the file and calls no method on the configuration, so an + // expectation this test does not consume describes nothing the code still does. + EasyMock.verify(mockConfiguration, mockConfigurationAdmin, mockBundleContext); } public void testDoConfigurationEventSavesUpdatedConfigurationWhenUsingCachingPersistence() throws Exception