[
https://issues.apache.org/jira/browse/FELIX-6850?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ankur Singla updated FELIX-6850:
--------------------------------
Description:
h2. Summary
{{ConfigurationAdminStarter.activate()}} and {{deactivate()}} mutate the shared
{{configurationManager}} field without mutual exclusion. When they run
concurrently on different threads, {{activate()}} can throw a
{{{}NullPointerException{}}}, and — even without the NPE — the two methods can
interleave so that a {{ConfigurationAdmin}} service is left orphaned (started
but no longer tracked) or registered twice. Because so many components wait on
Configuration Admin, this can wedge a large part of the framework.
h2. Affects Version/s
This is a latent defect, not a recent regression. The affected code was
introduced with the required-configuration-plugin gating in FELIX-6059 and
first shipped in {*}1.9.18{*}. It is unchanged in every release since, up to
and including *1.9.26* and current {{master}} (1.9.27-SNAPSHOT).
h2. Symptom
Observed as a framework error during bundle refresh / startup:
{code:java}
*ERROR* ... FrameworkEvent ERROR (java.lang.NullPointerException:
Cannot invoke
"org.apache.felix.cm.impl.ConfigurationManager.updateRegisteredConfigurationPlugins(String)"
because "this.configurationManager" is null)
{code}
h2. Root cause
{{ConfigurationAdminStarter}} exposes several lifecycle methods that all mutate
{{configurationManager}} / {{persistenceManager}} but share no common lock:
* Persistence-manager path: {{PersistenceManagerTracker}} calls
{{setPersistenceManager()}} / {{unsetPersistenceManager()}} while holding its
own {{holders}} monitor.
* Configuration-plugin path: {{RequiredConfigurationPluginTracker}} calls
{{updatePluginsSet()}} while holding no shared lock.
* Bundle stop: {{Activator.stop()}} -> {{DependencyTracker.stop()}} ->
{{{}deactivate(){}}}.
These run on independent threads, so {{activate()}} and {{deactivate()}} can
overlap. In {{activate()}} the {{configurationManager }}field is assigned and
then re-read on later lines:
{code:java}
configurationManager = new ConfigurationManager(pm, bundleContext);
this.startCoordinatorTracker();
final ServiceReference<ConfigurationAdmin> ref = configurationManager.start();
configurationManager.updateRegisteredConfigurationPlugins(this.registeredConfigurationPlugins);
// NPE
{code}
If {{deactivate()}} runs on another thread between the assignment and the
re-reads (it sets the field to {{{}null{}}}), the re-read throws the NPE
reported above. Even when the NPE is avoided, the lack of mutual exclusion
means an {{activate()}} that completes after a concurrent {{deactivate()}}
leaves a started-but-untracked ConfigurationAdmin (resource/thread leak), or a
second ConfigurationAdmin registered — an inconsistent state that never
recovers.
A secondary, related issue: the optional Coordinator {{ServiceTracker
}}callbacks in {{startCoordinatorTracker()}} dereference {{configurationManager
}}directly without a null check, which can NPE on coordinator service churn
during shutdown.
h2. When it triggers
Most easily reproduced when {{felix.cm.config.plugins}} requires one or more
ConfigurationPlugins (so activation is gated on plugin/PM availability) and
those services — or the Config Admin bundle itself — are refreshed/flap, e.g.
during a large bundle-refresh cycle or product upgrade. The concurrent
plugin/PM/stop callbacks drive {{activate()}} and {{deactivate()}} in parallel.
h2. Proposed fix
In {{{}ConfigurationAdminStarter{}}}:
# In {{{}activate(){}}}, capture the newly created {{ConfigurationManager}} in
a local variable and operate on that local (instead of re-reading the field),
so an interleaving {{deactivate()}} cannot cause an NPE.
# Serialize the lifecycle-transition methods ({{{}activate{}}},
{{{}deactivate{}}}, {{{}checkStart{}}}, {{{}updatePluginsSet{}}},
{{{}setPersistenceManager{}}}, {{{}unsetPersistenceManager{}}},
{{{}updateRegisteredConfigurationPlugins{}}}) so
{{{}activate{}}}/{{{}deactivate{}}} cannot interleave and leave an orphaned or
duplicate registration. This is consistent with the existing
PersistenceManagerTracker locking and is deadlock-safe: no path acquires the
starter monitor and then {{holders}} (only the reverse), and the update/event
threads and CA-registration listeners do not call back into the starter.
# Guard the Coordinator {{ServiceTracker}} callbacks against a null
{{{}configurationManager{}}}.
was:
h2. Summary
{{ConfigurationAdminStarter.activate()}} and {{deactivate()}} mutate the shared
{{configurationManager}} field without mutual exclusion. When they run
concurrently on different threads, {{activate()}} can throw a
{{{}NullPointerException{}}}, and — even without the NPE — the two methods can
interleave so that a {{ConfigurationAdmin}} service is left orphaned (started
but
no longer tracked) or registered twice. Because so many components wait on
Configuration Admin, this can wedge a large part of the framework.
h2. Affects Version/s
This is a latent defect, not a recent regression. The affected code was
introduced with the required-configuration-plugin gating in FELIX-6059 and first
shipped in {*}1.9.18{*}. It is unchanged in every release since, up to and
including
*1.9.26* and current {{master}} (1.9.27-SNAPSHOT).
h2. Symptom
Observed as a framework error during bundle refresh / startup:
{code:java}
*ERROR* ... FrameworkEvent ERROR (java.lang.NullPointerException:
Cannot invoke
"org.apache.felix.cm.impl.ConfigurationManager.updateRegisteredConfigurationPlugins(String)"
because "this.configurationManager" is null)
{code}
h2. Root cause
{{ConfigurationAdminStarter}} exposes several lifecycle methods that all mutate
{{configurationManager}} / {{persistenceManager}} but share no common lock:
* Persistence-manager path: {{PersistenceManagerTracker}} calls
{{setPersistenceManager()}} / {{unsetPersistenceManager()}} while holding its
own {{holders}} monitor.
* Configuration-plugin path: {{RequiredConfigurationPluginTracker}} calls
{{updatePluginsSet()}} while holding no shared lock.
* Bundle stop: {{Activator.stop()}} -> {{DependencyTracker.stop()}} ->
{{{}deactivate(){}}}.
These run on independent threads, so {{activate()}} and {{deactivate()}} can
overlap. In {{activate()}} the {{configurationManager}} field is assigned and
then re-read on later lines:
{code:java}
configurationManager = new ConfigurationManager(pm, bundleContext);
this.startCoordinatorTracker();
final ServiceReference<ConfigurationAdmin> ref = configurationManager.start();
configurationManager.updateRegisteredConfigurationPlugins(this.registeredConfigurationPlugins);
// NPE
{code}
If {{deactivate()}} runs on another thread between the assignment and the
re-reads (it sets the field to {{{}null{}}}), the re-read throws the NPE
reported
above. Even when the NPE is avoided, the lack of mutual exclusion means an
{{activate()}} that completes after a concurrent {{deactivate()}} leaves a
started-but-untracked ConfigurationAdmin (resource/thread leak), or a second
ConfigurationAdmin registered — an inconsistent state that never recovers.
A secondary, related issue: the optional Coordinator {{ServiceTracker}}
callbacks in {{startCoordinatorTracker()}} dereference {{configurationManager}}
directly without a null check, which can NPE on coordinator service churn during
shutdown.
h2. When it triggers
Most easily reproduced when {{felix.cm.config.plugins}} requires one or more
ConfigurationPlugins (so activation is gated on plugin/PM availability) and
those services — or the Config Admin bundle itself — are refreshed/flap, e.g.
during a large bundle-refresh cycle or product upgrade. The concurrent
plugin/PM/stop callbacks drive {{activate()}} and {{deactivate()}} in parallel.
h2. Proposed fix
In {{{}ConfigurationAdminStarter{}}}:
# In {{{}activate(){}}}, capture the newly created {{ConfigurationManager}} in
a
local variable and operate on that local (instead of re-reading the field), so
an interleaving {{deactivate()}} cannot cause an NPE.
# Serialize the lifecycle-transition methods ({{{}activate{}}},
{{{}deactivate{}}},
{{{}checkStart{}}}, {{{}updatePluginsSet{}}}, {{{}setPersistenceManager{}}},
{{{}unsetPersistenceManager{}}}, {{{}updateRegisteredConfigurationPlugins{}}})
so
{{{}activate{}}}/{{{}deactivate{}}} cannot interleave and leave an orphaned or
duplicate registration. This is consistent with the existing
PersistenceManagerTracker locking and is deadlock-safe: no path acquires the
starter monitor and then {{holders}} (only the reverse), and the update/event
threads and CA-registration listeners do not call back into the starter.
# Guard the Coordinator {{ServiceTracker}} callbacks against a null
{{{}configurationManager{}}}.
> ConfigAdmin: Race condition in ConfigurationAdminStarter activate/deactivate
> causes NPE and inconsistent Configuration Admin registration
> -----------------------------------------------------------------------------------------------------------------------------------------
>
> Key: FELIX-6850
> URL: https://issues.apache.org/jira/browse/FELIX-6850
> Project: Felix
> Issue Type: Bug
> Components: Configuration Admin
> Affects Versions: configadmin-1.9.26
> Reporter: Ankur Singla
> Priority: Major
> Fix For: configadmin-1.9.28
>
>
> h2. Summary
> {{ConfigurationAdminStarter.activate()}} and {{deactivate()}} mutate the
> shared {{configurationManager}} field without mutual exclusion. When they run
> concurrently on different threads, {{activate()}} can throw a
> {{{}NullPointerException{}}}, and — even without the NPE — the two methods
> can interleave so that a {{ConfigurationAdmin}} service is left orphaned
> (started but no longer tracked) or registered twice. Because so many
> components wait on Configuration Admin, this can wedge a large part of the
> framework.
> h2. Affects Version/s
> This is a latent defect, not a recent regression. The affected code was
> introduced with the required-configuration-plugin gating in FELIX-6059 and
> first shipped in {*}1.9.18{*}. It is unchanged in every release since, up to
> and including *1.9.26* and current {{master}} (1.9.27-SNAPSHOT).
> h2. Symptom
> Observed as a framework error during bundle refresh / startup:
> {code:java}
> *ERROR* ... FrameworkEvent ERROR (java.lang.NullPointerException:
> Cannot invoke
> "org.apache.felix.cm.impl.ConfigurationManager.updateRegisteredConfigurationPlugins(String)"
> because "this.configurationManager" is null)
> {code}
> h2. Root cause
> {{ConfigurationAdminStarter}} exposes several lifecycle methods that all
> mutate {{configurationManager}} / {{persistenceManager}} but share no common
> lock:
> * Persistence-manager path: {{PersistenceManagerTracker}} calls
> {{setPersistenceManager()}} / {{unsetPersistenceManager()}} while holding its
> own {{holders}} monitor.
> * Configuration-plugin path: {{RequiredConfigurationPluginTracker}} calls
> {{updatePluginsSet()}} while holding no shared lock.
> * Bundle stop: {{Activator.stop()}} -> {{DependencyTracker.stop()}} ->
> {{{}deactivate(){}}}.
> These run on independent threads, so {{activate()}} and {{deactivate()}} can
> overlap. In {{activate()}} the {{configurationManager }}field is assigned and
> then re-read on later lines:
> {code:java}
> configurationManager = new ConfigurationManager(pm, bundleContext);
> this.startCoordinatorTracker();
> final ServiceReference<ConfigurationAdmin> ref = configurationManager.start();
> configurationManager.updateRegisteredConfigurationPlugins(this.registeredConfigurationPlugins);
> // NPE
> {code}
> If {{deactivate()}} runs on another thread between the assignment and the
> re-reads (it sets the field to {{{}null{}}}), the re-read throws the NPE
> reported above. Even when the NPE is avoided, the lack of mutual exclusion
> means an {{activate()}} that completes after a concurrent {{deactivate()}}
> leaves a started-but-untracked ConfigurationAdmin (resource/thread leak), or
> a second ConfigurationAdmin registered — an inconsistent state that never
> recovers.
> A secondary, related issue: the optional Coordinator {{ServiceTracker
> }}callbacks in {{startCoordinatorTracker()}} dereference
> {{configurationManager }}directly without a null check, which can NPE on
> coordinator service churn during shutdown.
> h2. When it triggers
> Most easily reproduced when {{felix.cm.config.plugins}} requires one or more
> ConfigurationPlugins (so activation is gated on plugin/PM availability) and
> those services — or the Config Admin bundle itself — are refreshed/flap, e.g.
> during a large bundle-refresh cycle or product upgrade. The concurrent
> plugin/PM/stop callbacks drive {{activate()}} and {{deactivate()}} in
> parallel.
> h2. Proposed fix
> In {{{}ConfigurationAdminStarter{}}}:
> # In {{{}activate(){}}}, capture the newly created {{ConfigurationManager}}
> in a local variable and operate on that local (instead of re-reading the
> field), so an interleaving {{deactivate()}} cannot cause an NPE.
> # Serialize the lifecycle-transition methods ({{{}activate{}}},
> {{{}deactivate{}}}, {{{}checkStart{}}}, {{{}updatePluginsSet{}}},
> {{{}setPersistenceManager{}}}, {{{}unsetPersistenceManager{}}},
> {{{}updateRegisteredConfigurationPlugins{}}}) so
> {{{}activate{}}}/{{{}deactivate{}}} cannot interleave and leave an orphaned
> or duplicate registration. This is consistent with the existing
> PersistenceManagerTracker locking and is deadlock-safe: no path acquires the
> starter monitor and then {{holders}} (only the reverse), and the update/event
> threads and CA-registration listeners do not call back into the starter.
> # Guard the Coordinator {{ServiceTracker}} callbacks against a null
> {{{}configurationManager{}}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
