[
https://issues.apache.org/jira/browse/FELIX-6812?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18115641#comment-18115641
]
Kevan Jahanshahi commented on FELIX-6812:
-----------------------------------------
The filter this change introduced is built from the pid by concatenation, and a
pid is not a filter-safe value.
{code:java}
listConfigurations("(service.pid=" + configurationEvent.getPid() + ")");
{code}
{{findExistingConfiguration}} escapes its own filter value through
{{escapeFilterValue}}, nine lines below in the same class. The new call site
does not.
Two failures follow, and both are silent.
* 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}}. The catch
reports it as "Unable to save configuration", so the configuration is never
written back to its file.
I also found that {{escapeFilterValue}} leaves the backslash alone, although
the OSGi filter grammar reserves it. That defect predates this issue, and it
affects {{findExistingConfiguration}} as well. The escaping added for the pid
is incomplete without the backslash case, so my patch fixes both and escapes
the backslash first. Tell me if you would rather see the backslash in an issue
of its own, and I will split the patch.
One more observation about this change, which needs no fix and is worth
recording. {{listConfigurations}} returns only the configurations the caller
may see, and {{getConfiguration(pid, "?")}} did not filter that way. A
configuration bound to another bundle's location therefore stops being written
back to its file once a SecurityManager runs. See Configuration Admin 104.13.3.
Neither early return logs anything, so the skip leaves no trace. My patch logs
the empty result at debug level.
I have a patch with two tests, one for a pid holding an asterisk and one for a
pid holding a backslash. Each test fails when its escape is removed. Since
3.7.5 is not released yet, I can open the pull request against this issue as a
follow-up. Tell me if you prefer a new issue instead.
> FileInstall re-creates deleted ConfigAdmin configurations when processing
> stale CM_UPDATED events
> -------------------------------------------------------------------------------------------------
>
> Key: FELIX-6812
> URL: https://issues.apache.org/jira/browse/FELIX-6812
> Project: Felix
> Issue Type: Bug
> Components: Configuration Admin, File Install
> Affects Versions: fileinstall-3.7.4, configadmin-1.9.26
> Reporter: Felix Marx
> Priority: Major
> Fix For: fileinstall-3.7.5
>
>
> When an in-memory configuration (no {{{}felix.fileinstall.filename{}}}) is
> deleted via {{{}Configuration.delete(){}}}, and a stale {{CM_UPDATED}} event
> for the same PID is later delivered to {{{}ConfigInstaller{}}}, FileInstall
> calls {{ConfigurationAdmin.getConfiguration(pid, "?")}} in the {{CM_UPDATED}}
> handler. Since the configuration had been deleted, ConfigAdmin recreates a
> new empty configuration, effectively resurrecting it with no properties.
> FileInstall then ignores the event (no {{{}felix.fileinstall.filename{}}}),
> but the configuration remains in ConfigAdmin’s cache.
> This means FileInstall can recreate deleted configurations which it does not
> own and which are not backed by any file.
> Attached you will find 2 stacktraces that we have in our code:
> Initial deletion (still part of the original CM_UPDATED)
> {code:java}
> Breakpoint reached at
> org.apache.felix.cm.impl.ConfigurationManager.fireConfigurationEvent(ConfigurationManager.java:667)
> at
> org.apache.felix.cm.impl.ConfigurationManager.deleted(ConfigurationManager.java:598)
> at
> org.apache.felix.cm.impl.ConfigurationImpl.delete(ConfigurationImpl.java:276)
> at
> org.apache.felix.cm.impl.ConfigurationAdapter.delete(ConfigurationAdapter.java:184)
> at
> com.instana.agent.main.impl.ConfigurationServiceImpl.deleteAndRemoveFromInventory(ConfigurationServiceImpl.java:306)
> at
> com.instana.agent.main.impl.ConfigurationServiceImpl.deleteAndRemoveFromInventoryBecauseProcessDied(ConfigurationServiceImpl.java:289)
> at
> com.instana.agent.main.impl.ConfigurationServiceImpl.deleteProcessDependentConfiguration(ConfigurationServiceImpl.java:164)
> at
> com.instana.agent.process.handling.sensor.SensorUtilImpl.remove(SensorUtilImpl.java:288)
> at com.instana.agent.process.sensor.Process.getStats(Process.java:358)
> at
> com.instana.agent.process.sensor.Process$$Lambda$901.1176822721.run(Unknown
> Source:-1) at
> com.instana.agent.api.ObservableRunnable.run(ObservableRunnable.java:65)
> at
> com.instana.agent.util.ErrorLoggingRunnable.run(ErrorLoggingRunnable.java:33)
> at
> com.instana.agent.task.orchestrator.api.ExecutionPipeline.execute(ExecutionPipeline.java:247)
> at
> com.instana.agent.task.orchestrator.api.ExecutionPipeline.lambda$wrapWithErrorLogging$5(ExecutionPipeline.java:357)
> at
> com.instana.agent.task.orchestrator.api.ExecutionPipeline$$Lambda$500.1618304857.run(Unknown
> Source:-1) at
> com.instana.agent.util.ErrorLoggingRunnable.run(ErrorLoggingRunnable.java:33)
> at
> com.instana.agent.task.orchestrator.impl.ScheduledFutureCallbackTask.run(ScheduledFutureCallbackTask.java:66)
> at
> java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
> at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305) at
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
> at
> com.instana.agent.task.orchestrator.impl.CallbackDecoratedRunnableScheduledFuture.run(CallbackDecoratedRunnableScheduledFuture.java:53)
> at
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
> at
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
> at
> io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
> at java.lang.Thread.run(Thread.java:829)
> # Content of the event
> 2 - CM event Fire ConfigurationEvent:
> pid=com.instana.agent.memcached.sensor.Memcached.8ba1c3ef-5823-4d51-9ba5-53bbceaed49c
> {code}
> Re-creation of the configuration:
> {code:java}
> Breakpoint reached at
> org.apache.felix.cm.impl.ConfigurationManager.cacheConfiguration(ConfigurationManager.java:328)
> at
> org.apache.felix.cm.impl.ConfigurationManager.createConfiguration(ConfigurationManager.java:516)
> at
> org.apache.felix.cm.impl.ConfigurationAdminImpl.getConfiguration(ConfigurationAdminImpl.java:167)
> at
> org.apache.felix.fileinstall.internal.ConfigInstaller.doConfigurationEvent(ConfigInstaller.java:240)
> at
> org.apache.felix.fileinstall.internal.ConfigInstaller.configurationEvent(ConfigInstaller.java:222)
> at
> org.apache.felix.cm.impl.ConfigurationManager$FireConfigurationEvent.sendEvent(ConfigurationManager.java:1721)
> at
> org.apache.felix.cm.impl.ConfigurationManager$FireConfigurationEvent.run(ConfigurationManager.java:1662)
> at org.apache.felix.cm.impl.UpdateThread.run0(UpdateThread.java:122)
> at org.apache.felix.cm.impl.UpdateThread.run(UpdateThread.java:84) at
> java.lang.Thread.run(Thread.java:829)configurations.size: 45
> # content of the event
> Addition of
> com.instana.agent.memcached.sensor.Memcached.8ba1c3ef-5823-4d51-9ba5-53bbceaed49c{code}
>
> We use Karaf 4.2.16, with a self compiled version of felix fileinstall 3.7.5.
> The issue is still present in the latest version.
> We also know that by allowing to call *delete* within the *activate* method
> of our components, we can trigger this issue with a higher percentage.
> Nevertheless fileinstall should not be able to create a configuration in
> configadmin when checking if a config should be written back.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
