Skip to content

Commit b53d87e

Browse files
mohitc1Mohitrpdome
authored
Fix ABBA deadlock between AzureActiveDirectory and AzureActiveDirectoryAuthority class monitors, Fixes AB#3578299 (#3082)
## Summary Fixes an ABBA deadlock between `AzureActiveDirectory.class` and `AzureActiveDirectoryAuthority.class` monitors, discovered during ANR investigation on Microsoft Authenticator (Broker v16.0.0). Under high concurrency (multiple apps calling into Broker simultaneously), two threads could deadlock: - **Path A** (`isKnownAuthority`): `sLock` → `authority.getAuthorityURL()` → `AzureActiveDirectoryAuthority.class` → `AzureActiveDirectory.class` - **Path B** (`ensureCloudDiscoveryForAuthority`): `AzureActiveDirectory.class` → `authority.getAuthorityURL()` → `AzureActiveDirectoryAuthority.class` ## Changes ### Source (3 files) **Authority.java** - Extract `authority.getAuthorityURL()` outside `synchronized(sLock)` in `isKnownAuthority()` - Add null check for `authorityUrl` - Improve error handling in `getKnownAuthorityResult()`: propagate original discovery exception error code instead of replacing with `UNKNOWN_AUTHORITY` - Make `KnownAuthorityResult` fields `final` **AzureActiveDirectory.java** - Remove `synchronized` from read-only `ConcurrentHashMap` methods: `hasCloudHost`, `getAzureActiveDirectoryCloud`, `getAzureActiveDirectoryCloudFromHostName`, `isValidCloudHost` - Extract `authority.getAuthorityURL()` outside synchronized scope in `ensureCloudDiscoveryForAuthority(Authority)` **AzureActiveDirectoryAuthority.java** - Remove `synchronized` from `private static getAzureActiveDirectoryCloud()` ### Tests (2 files) **AzureActiveDirectoryTest.kt** — 5 concurrency regression tests: 1. ABBA deadlock between `isKnownAuthority` and `ensureCloudDiscovery` 2. Concurrent reads do not block (ConcurrentHashMap safety) 3. Concurrent `getAuthorityURL()` calls 4. Concurrent reader/writer scenarios 5. High-concurrency stress test with multiple authorities **AuthorityKnownAuthorityTest.kt** — expanded: - Sovereign cloud recognition (Bleu, Delos, GovSG) - Null authority / null authorityUrl handling - Error code propagation (IO_ERROR, MALFORMED_URL, UNKNOWN_AUTHORITY) - Developer-configured authority with discovery failure - Pre-seeded metadata fallback ## Linked Work Items [AB#3578299](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3578299) --------- Co-authored-by: Mohit <mchand@microsoft.com> Co-authored-by: Dome Pongmongkol <19558668+rpdome@users.noreply.github.com>
1 parent d87bc2e commit b53d87e

File tree

6 files changed

+526
-70
lines changed

6 files changed

+526
-70
lines changed

‎changelog.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
vNext
22
----------
3+
- [PATCH] Fix ABBA deadlock between AzureActiveDirectory and AzureActiveDirectoryAuthority class monitors by extracting polymorphic getAuthorityURL() calls outside synchronized scopes and removing unnecessary synchronized from ConcurrentHashMap read-only methods (#3082)
34
- [PATCH] Optimize AcquireTokenSilent save path: replace keySet() decrypt-all with in-memory map lookup in removeAccount()/removeCredential(), add telemetry for deleteAccessTokensWithIntersectingScopes, and remove unused elapsed_time_save_account_shared_preferences attribute (#3074)
45
- [MINOR] Add DeviceRegistrationClientApplication as public API for OneAuth device registration with mandatory correlationId, DeviceState and DrsDiscoveryEndpoint enums (#3073)
56
- [MINOR] Move device registration protocol types, domain types, controller, and packer from broker to common to enable OneAuth device registration support (#3066)

‎common4j/src/main/com/microsoft/identity/common/java/authorities/Authority.java‎

Lines changed: 49 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -318,40 +318,52 @@ public static void addKnownAuthorities(List<Authority> authorities) {
318318
}
319319

320320
/**
321-
* Authorities are either known by the developer and communicated to the library via configuration or they
322-
* are known to Microsoft based on the list of clouds returned from:
321+
* Clears all known authorities. For test use only.
322+
*/
323+
// Visible for testing
324+
static void clearKnownAuthorities() {
325+
synchronized (sLock) {
326+
knownAuthorities.clear();
327+
}
328+
}
329+
330+
/**
331+
* Authorities are either known by the developer and communicated to the library via
332+
* configuration, or they are known to Microsoft based on the cloud discovery metadata cache.
323333
*
324334
* @param authority Authority to check against.
325335
* @return True if the authority is known to Microsoft or defined in the configuration.
326336
*/
327337
public static boolean isKnownAuthority(Authority authority) {
328-
final String methodName = ":isKnownAuthority";
338+
final String methodTag = TAG + ":isKnownAuthority";
329339
boolean knownToDeveloper = false;
330340
boolean knownToMicrosoft;
331341

332342
if (authority == null) {
333-
Logger.warn(
334-
TAG + methodName,
335-
"Authority is null"
336-
);
343+
Logger.warn(methodTag, "Authority is null");
344+
return false;
345+
}
346+
347+
// Resolve the authority URL outside any lock to avoid calling a potentially-
348+
// synchronized polymorphic method (e.g. AzureActiveDirectoryAuthority.getAuthorityURL())
349+
// while holding sLock, which could create a lock-ordering inversion.
350+
final URL authorityUrl = authority.getAuthorityURL();
351+
352+
if (authorityUrl == null) {
353+
Logger.warn(methodTag, "Authority URL is null");
337354
return false;
338355
}
339356

340-
if (BuildConfig.ALLOW_ONEBOX_AUTHORITIES && AzureActiveDirectoryEnvironment.ONEBOX_AUTHORITY.equals(authority.getAuthorityURL().getAuthority())) {
357+
if (BuildConfig.ALLOW_ONEBOX_AUTHORITIES && AzureActiveDirectoryEnvironment.ONEBOX_AUTHORITY.equals(authorityUrl.getAuthority())) {
341358
return true; // onebox authorities are always considered to be known.
342359
}
343360

344-
//Check if authority was added to configuration
345361
synchronized (sLock) {
346362
for (final Authority currentAuthority : knownAuthorities) {
347363
if (currentAuthority.mAuthorityUrlString != null &&
348-
authority.getAuthorityURL() != null &&
349-
authority.getAuthorityURL().getAuthority() != null &&
364+
authorityUrl.getAuthority() != null &&
350365
currentAuthority.mAuthorityUrlString.toLowerCase(Locale.ROOT).contains(
351-
authority
352-
.getAuthorityURL()
353-
.getAuthority()
354-
.toLowerCase(Locale.ROOT))) {
366+
authorityUrl.getAuthority().toLowerCase(Locale.ROOT))) {
355367
knownToDeveloper = true;
356368
break;
357369
}
@@ -360,19 +372,14 @@ public static boolean isKnownAuthority(Authority authority) {
360372

361373
// Check whether the authority is known to Microsoft or not. Microsoft can recognize authorities that exist within public clouds.
362374
// Microsoft does not maintain a list of B2C authorities or a list of ADFS or 3rd party authorities (issuers).
363-
knownToMicrosoft = AzureActiveDirectory.hasCloudHost(authority.getAuthorityURL());
375+
knownToMicrosoft = AzureActiveDirectory.hasCloudHost(authorityUrl);
364376

365377
final boolean isKnown = (knownToDeveloper || knownToMicrosoft);
366378

367-
Logger.verbose(
368-
TAG + methodName,
369-
"Authority is known to developer? [" + knownToDeveloper + "]"
370-
);
371-
372-
Logger.verbose(
373-
TAG + methodName,
374-
"Authority is known to Microsoft? [" + knownToMicrosoft + "]"
375-
);
379+
Logger.verbose(methodTag,
380+
"Authority is known to developer? [" + knownToDeveloper + "]");
381+
Logger.verbose(methodTag,
382+
"Authority is known to Microsoft? [" + knownToMicrosoft + "]");
376383

377384
return isKnown;
378385
}
@@ -383,37 +390,43 @@ public static KnownAuthorityResult getKnownAuthorityResult(Authority authority)
383390
methodTag,
384391
"Getting known authority result..."
385392
);
386-
ClientException clientException = null;
387-
boolean known = false;
388393

394+
ClientException discoveryException = null;
389395
try {
390396
AzureActiveDirectory.ensureCloudDiscoveryForAuthority(authority);
391397
Logger.info(methodTag, "Cloud discovery complete.");
392398
} catch (final ClientException ex) {
393399
// Cloud discovery failed (e.g. network error).
394400
// Log but continue — the authority may still be known via hardcoded
395401
// metadata or developer configuration.
402+
discoveryException = ex;
396403
Logger.warn(methodTag,
397404
"Cloud discovery failed, will check hardcoded/configured authorities. Error: "
398405
+ ex.getErrorCode());
399406
}
400407

401-
if (!isKnownAuthority(authority)) {
402-
clientException = new ClientException(
408+
if (isKnownAuthority(authority)) {
409+
Logger.info(methodTag, "Authority is known.");
410+
return new KnownAuthorityResult(true, null);
411+
}
412+
413+
// Authority is not known via any source.
414+
if (discoveryException != null) {
415+
// Discovery failed — propagate the original error
416+
return new KnownAuthorityResult(false, discoveryException);
417+
} else {
418+
// Discovery succeeded but authority is not in any known list.
419+
final ClientException clientException = new ClientException(
403420
ClientException.UNKNOWN_AUTHORITY,
404421
"Provided authority is not known. MSAL will only make requests to known authorities"
405422
);
406-
} else {
407-
Logger.info(methodTag, "Cloud is known.");
408-
known = true;
423+
return new KnownAuthorityResult(false, clientException);
409424
}
410-
411-
return new KnownAuthorityResult(known, clientException);
412425
}
413426

414427
public static class KnownAuthorityResult {
415-
private boolean mKnown;
416-
private ClientException mClientException;
428+
private final boolean mKnown;
429+
private final ClientException mClientException;
417430

418431
KnownAuthorityResult(boolean known, ClientException exception) {
419432
mKnown = known;

‎common4j/src/main/com/microsoft/identity/common/java/authorities/AzureActiveDirectoryAuthority.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public class AzureActiveDirectoryAuthority extends Authority {
7171

7272
/** Gets {@link AzureActiveDirectoryCloud}, if the cloud metadata is already initialized. */
7373
@Nullable
74-
private static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(
74+
private static AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(
7575
@NonNull final AzureActiveDirectoryAudience audience) {
7676
final String methodName = ":getAzureActiveDirectoryCloud";
7777

@@ -175,7 +175,7 @@ public OAuth2Strategy createOAuth2Strategy(@NonNull final OAuth2StrategyParamete
175175
* @return true if both authorities belong to same cloud, otherwise false.
176176
*/
177177
//@WorkerThread
178-
public synchronized boolean isSameCloudAsAuthority(@NonNull final AzureActiveDirectoryAuthority authorityToCheck)
178+
public boolean isSameCloudAsAuthority(@NonNull final AzureActiveDirectoryAuthority authorityToCheck)
179179
throws ClientException {
180180
// Cloud discovery is needed to make sure that we have preferred_network_host_name to cloud aliases mappings
181181
AzureActiveDirectory.ensureCloudDiscoveryForAuthority(this);

‎common4j/src/main/com/microsoft/identity/common/java/providers/microsoft/azureactivedirectory/AzureActiveDirectory.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -164,12 +164,13 @@ public AzureActiveDirectoryOAuth2Strategy createOAuth2Strategy(@NonNull final Az
164164
return new AzureActiveDirectoryOAuth2Strategy(config, parameters);
165165
}
166166

167-
public static synchronized boolean hasCloudHost(@NonNull final URL authorityUrl) {
167+
public static boolean hasCloudHost(@NonNull final URL authorityUrl) {
168168
return sAadClouds.containsKey(authorityUrl.getHost().toLowerCase(Locale.US));
169169
}
170170

171-
public static synchronized boolean isValidCloudHost(@NonNull final URL authorityUrl) {
172-
return hasCloudHost(authorityUrl) && getAzureActiveDirectoryCloud(authorityUrl).isValidated();
171+
public static boolean isValidCloudHost(@NonNull final URL authorityUrl) {
172+
final AzureActiveDirectoryCloud cloud = getAzureActiveDirectoryCloud(authorityUrl);
173+
return cloud != null && cloud.isValidated();
173174
}
174175

175176
public static synchronized void setEnvironment(@NonNull final Environment environment) {
@@ -191,15 +192,15 @@ public static synchronized Environment getEnvironment() {
191192
* @param authorityUrl URL
192193
* @return AzureActiveDirectoryCloud
193194
*/
194-
public static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(@NonNull final URL authorityUrl) {
195+
public static AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(@NonNull final URL authorityUrl) {
195196
return sAadClouds.get(authorityUrl.getHost().toLowerCase(Locale.US));
196197
}
197198

198199
/**
199200
* @param preferredCacheHostName String
200201
* @return AzureActiveDirectoryCloud
201202
*/
202-
public static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloudFromHostName(@NonNull final String preferredCacheHostName) {
203+
public static AzureActiveDirectoryCloud getAzureActiveDirectoryCloudFromHostName(@NonNull final String preferredCacheHostName) {
203204
return sAadClouds.get(preferredCacheHostName.toLowerCase(Locale.US));
204205
}
205206

@@ -368,11 +369,16 @@ public static synchronized void ensureCloudDiscovery() throws ClientException {
368369
*
369370
* @param authority The authority whose URL determines the discovery endpoint, or null to use the default.
370371
*/
371-
public static synchronized void ensureCloudDiscoveryForAuthority(@Nullable final Authority authority)
372+
public static void ensureCloudDiscoveryForAuthority(@Nullable final Authority authority)
372373
throws ClientException {
373-
ensureCloudDiscoveryForAuthority(
374-
authority != null ? authority.getAuthorityURL() : null
375-
);
374+
// Resolve the URL outside the synchronized scope to avoid deadlock:
375+
// getAuthorityURL() on AzureActiveDirectoryAuthority acquires
376+
// AzureActiveDirectoryAuthority.class then AzureActiveDirectory.class,
377+
// while ensureCloudDiscoveryForAuthority(URL) acquires AzureActiveDirectory.class.
378+
// Calling getAuthorityURL() while holding AzureActiveDirectory.class would
379+
// create a lock-ordering inversion with any concurrent getAuthorityURL() caller.
380+
final URL authorityUrl = authority != null ? authority.getAuthorityURL() : null;
381+
ensureCloudDiscoveryForAuthority(authorityUrl);
376382
}
377383

378384
/**

0 commit comments

Comments
 (0)