[
https://issues.apache.org/jira/browse/FELIX-6850?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18099383#comment-18099383
]
Ankur Singla commented on FELIX-6850:
-------------------------------------
Raised a PR: [https://github.com/apache/felix-dev/pull/537]
> 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)
