From 9325c984702c9108d7890f1ba4fc76f3861f7c80 Mon Sep 17 00:00:00 2001 From: "russell@unturf.com" Date: Sun, 29 Mar 2026 21:09:07 -0400 Subject: [PATCH] =?UTF-8?q?moad-sweep:=209=20initial=20findings=20?= =?UTF-8?q?=E2=80=94=20MOAD-0005=20(Hungry=20Regex)=20x1,=20MOAD-0006=20(T?= =?UTF-8?q?hundering=20Herd)=20x8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MOAD-0005 candidates (ReDoS): - bleach-MOADX-0001: sanitize_css O(2^N) — 71-char input causes 12s hang HIGH MOAD-0006 candidates (Thundering Herd / cache stampede): - hibernate-orm-MOADX-0001: QueryInterpretationCacheStandardImpl HQL plan cache HIGH - elasticsearch-MOADX-0001: EnrichCache "intentionally non-locking" enrich search HIGH - hadoop-MOADX-0001: FederationJCache/FederationCaffeineCache YARN default config HIGH - celery-MOADX-0001: BaseBackend.get_task_meta chord fan-in Redis stampede HIGH - traefik-MOADX-0001: CNAMEFlatten data race + N*30s DNS stampede HIGH - kafka-MOADX-0001: CachedConnectors.lookup classloader scan MEDIUM - spring-MOADX-0001: AbstractFallbackCacheOperationSource @Cacheable cold start MEDIUM - django-MOADX-0001: cached.Loader.get_template template compile MEDIUM CLEAN: Consul (singleflight), Kubernetes (RWMutex), Prometheus (per-scraper), Pulsar (AsyncLoadingCache), Cassandra (LoadingCache), Flink (CAS loop), ZooKeeper (synchronized), Airflow (hand-rolled singleflight) --- ...-MOADX-0001-sanitize-css-gauntlet-redos.md | 105 ++++++++++++ ...MOADX-0001-get-task-meta-cache-stampede.md | 137 +++++++++++++++ ...DX-0001-cached-loader-template-stampede.md | 144 ++++++++++++++++ ...search-MOADX-0001-enrich-cache-stampede.md | 111 +++++++++++++ ...op-MOADX-0001-federation-cache-stampede.md | 113 +++++++++++++ ...001-query-interpretation-cache-stampede.md | 156 ++++++++++++++++++ ...0001-connect-cached-connectors-stampede.md | 93 +++++++++++ ...allback-cache-operation-source-stampede.md | 138 ++++++++++++++++ ...MOADX-0001-cname-flatten-cache-stampede.md | 114 +++++++++++++ 9 files changed, 1111 insertions(+) create mode 100644 defects/bleach/patch/bleach-MOADX-0001-sanitize-css-gauntlet-redos.md create mode 100644 defects/celery/patch/celery-MOADX-0001-get-task-meta-cache-stampede.md create mode 100644 defects/django/patch/django-MOADX-0001-cached-loader-template-stampede.md create mode 100644 defects/elasticsearch/patch/elasticsearch-MOADX-0001-enrich-cache-stampede.md create mode 100644 defects/hadoop/patch/hadoop-MOADX-0001-federation-cache-stampede.md create mode 100644 defects/hibernate-orm/patch/hibernate-orm-MOADX-0001-query-interpretation-cache-stampede.md create mode 100644 defects/kafka/patch/kafka-MOADX-0001-connect-cached-connectors-stampede.md create mode 100644 defects/spring/patch/spring-MOADX-0001-fallback-cache-operation-source-stampede.md create mode 100644 defects/traefik/patch/traefik-MOADX-0001-cname-flatten-cache-stampede.md diff --git a/defects/bleach/patch/bleach-MOADX-0001-sanitize-css-gauntlet-redos.md b/defects/bleach/patch/bleach-MOADX-0001-sanitize-css-gauntlet-redos.md new file mode 100644 index 000000000..7f8d3a94b --- /dev/null +++ b/defects/bleach/patch/bleach-MOADX-0001-sanitize-css-gauntlet-redos.md @@ -0,0 +1,105 @@ +# UNDF: (pending) +# bleach-MOADX-0001: BleachSanitizerFilter.sanitize_css — ReDoS O(2^N) on style attribute + +## MOAD-0005 Candidate — The Hungry Regex + +| Field | Value | +|-------|-------| +| ID | bleach-MOADX-0001 | +| Severity | HIGH | +| Ecosystem | bleach (Python HTML sanitizer) | +| File | `bleach/sanitizer.py` | +| Lines | 553–558 | +| Pattern | `^([-/:,#%.'"\sa-zA-Z0-9!]\|\w-\w\|'[\s\w]+'\s*\|"[\s\w]+"\|\([\d,%\.\s]+\))*$` | +| Trigger | `style="a-a-a-a-...-a-@"` — 71 chars causes ~12-second hang | +| Input vector | HTML `style` attribute value from user-submitted content | +| Input length limit | none | + +## Defect + +`bleach/sanitizer.py`, function `sanitize_css`, lines 553–558: + +```python +parts = style.split(';') +gauntlet = re.compile( + r"""^([-/:,#%.'"\sa-zA-Z0-9!]|\w-\w|'[\s\w]+'\s*|"[\s\w]+"|\([\d,%\.\s]+\))*$""" +) + +for part in parts: + if not gauntlet.match(part): + return '' +``` + +The pattern `^(A|B|C|D|E)*$` where: +- `A` = `[-/:,#%.'"\sa-zA-Z0-9!]` — single char (broad set including `-`) +- `B` = `\w-\w` — three chars: word-hyphen-word + +The alternation `A|B` creates exponential ambiguity: a string like `a-a-a-a-` can +be parsed as `[a][-][a][-]` (all via `A`) or `[a-a][-][a-]` (via `B` then `A`) or +countless other combinations. For each prefix position, the regex engine must +explore both interpretations. + +When the input does NOT match (the trailing `$` fails), Python's NFA backtracks +through all O(2^N) parse trees, where N is the number of `a-` repetitions. + +**Measurement (Python 3.x, bleach 3.0.0):** + +| Input length | Time | +|---|---| +| 41 chars (`a-`×20 + `@`) | 0.011s | +| 51 chars (`a-`×25 + `@`) | 0.11s | +| 61 chars (`a-`×30 + `@`) | 0.97s | +| 71 chars (`a-`×35 + `@`) | 12.8s | + +Growth factor: ~10× per 10 chars = O(2^N). + +**Attack payload:** +``` +style="a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-a-@" +``` +71-char style attribute causes a ~12-second Python thread hang. + +Triggering characters for the non-matching terminal: `@`, `$`, `^`, `~`, `` ` ``, +`|`, `\`, `{`, `}`, `<`, `>` — all realistic HTML/CSS chars. + +**Input path:** HTML user content → `bleach.clean()` → `BleachSanitizerFilter.sanitize_token()` → +`sanitize_css()` → `gauntlet.match(part)`. + +## Fix + +The ambiguity is between `A` matching `-` as a single character and `B` matching +`\w-\w` as a three-character sequence. Eliminating the single-char `-` from group +`A` and keeping it only in `B` removes the ambiguity, OR use atomic grouping +(Python 3.11+ via `re` module possessive `(?>...)`), OR replace with a proper +CSS tokenizer. + +**Minimal fix — remove `-` from the single-char group:** +```python +gauntlet = re.compile( + r"""^([-/:,#%.'"\sa-zA-Z0-9!]|(?> searchResponseFetcher, + ActionListener>> listener +) { + // intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition. + long cacheStart = relativeNanoTimeProvider.getAsLong(); + var cacheKey = new CacheKey(projectId, enrichIndex, lookupValue, maxMatches); + List> response = get(cacheKey); // (1) all N threads: miss + long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart; + if (response != null) { + hitsTimeInNanos.add(cacheRequestTime); + listener.onResponse(response); + } else { + final long retrieveStart = relativeNanoTimeProvider.getAsLong(); + searchResponseFetcher.accept(ActionListener.wrap(resp -> { // (2) N×search queries + CacheValue cacheValue = toCacheValue(resp); + put(cacheKey, cacheValue); // (3) N redundant puts + // ... + }, listener::onFailure)); + } +} +``` + +The comment "it's OK if we re-put the same key/value" is wrong for high-cardinality enrich lookups. It is not OK when: +1. **The enrich index is large**: each search hits the Lucene index for that enrich data, consuming heap and I/O +2. **The cache key has high cardinality** (e.g., IP address lookup in a geo-enrich policy): TTL expiry under ingestion load causes N threads to all fire searches for the same IP simultaneously +3. **The search requires network hops**: in a multi-node cluster the enrich coordinator proxies searches to the shard owner (see `EnrichCoordinatorProxyAction`) — N concurrent searches all go to the same shard + +### Ingest pipeline throughput amplification + +Enrich processors are designed to be on the hot path for bulk indexing. At 10,000 documents/second with a TTL-expired cache entry, the cluster can receive 10,000 simultaneous search queries for the same lookup value before the first one completes and populates the cache. + +## Impact + +- Enrich index shard becomes saturated with redundant identical searches +- Ingest throughput drops dramatically during cache cold start or TTL expiry +- In worst case (hot key + small TTL), the stampede is continuous: searches pile up before cache is populated, the pipeline backs up, and ES node heap fills with queued search contexts +- Kibana's `_enrich/stats` will show abnormally high `misses_time_in_nanos` with ratio misses/hits > 1 + +## Fix + +Use a `ConcurrentHashMap` of in-flight `CompletableFuture`s to coalesce concurrent misses for the same key — the same pattern as Pulsar's `BookkeeperSchemaStorage.readSchemaOperations`: + +```java +private final ConcurrentHashMap>>> inflight = + new ConcurrentHashMap<>(); + +public void computeIfAbsent( + ProjectId projectId, + String enrichIndex, + Object lookupValue, + int maxMatches, + Consumer> searchResponseFetcher, + ActionListener>> listener +) { + var cacheKey = new CacheKey(projectId, enrichIndex, lookupValue, maxMatches); + List> cached = get(cacheKey); + if (cached != null) { + listener.onResponse(cached); + return; + } + // Coalesce: only one search fires per key; all other waiters share the future + CompletableFuture>> future = inflight.computeIfAbsent(cacheKey, k -> { + CompletableFuture>> f = new CompletableFuture<>(); + searchResponseFetcher.accept(ActionListener.wrap(resp -> { + CacheValue cacheValue = toCacheValue(resp); + put(cacheKey, cacheValue); + inflight.remove(cacheKey); + f.complete(deepCopy(cacheValue.hits, false)); + }, ex -> { + inflight.remove(cacheKey); + f.completeExceptionally(ex); + })); + return f; + }); + future.whenComplete((result, ex) -> { + if (ex != null) { + listener.onFailure(new RuntimeException(ex)); + } else { + listener.onResponse(result); + } + }); +} +``` + +Alternatively, replace the custom `Cache` with Caffeine `AsyncLoadingCache` which provides this deduplication automatically. diff --git a/defects/hadoop/patch/hadoop-MOADX-0001-federation-cache-stampede.md b/defects/hadoop/patch/hadoop-MOADX-0001-federation-cache-stampede.md new file mode 100644 index 000000000..c57b0b7be --- /dev/null +++ b/defects/hadoop/patch/hadoop-MOADX-0001-federation-cache-stampede.md @@ -0,0 +1,113 @@ +# UNDF: (pending) +# hadoop-MOADX-0001: FederationJCache/FederationCaffeineCache — cache stampede on YARN Federation state store + +## MOAD-0006 Candidate — The Thundering Herd + +| Field | Value | +|-------|-------| +| ID | hadoop-MOADX-0001 | +| Severity | HIGH | +| Ecosystem | Apache Hadoop | +| Files | `hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/cache/FederationJCache.java` lines 101–138; `FederationCaffeineCache.java` lines 89–125 | +| Cache type | YARN Federation state store (sub-cluster registry, policy configurations, application home sub-cluster) | +| Pattern | get → null → stateStore.getXxx() → put (no lock, no CAS) | +| Trigger | N concurrent Router requests all miss the same cache key → N×stateStore calls hit ZooKeeper/SQL simultaneously | +| Hot path | YARN Router per-request path: every application submission, every allocation, every cluster query goes through `FederationStateStoreFacade.getSubClusters()` or `getApplicationHomeSubCluster()` | + +## Defect + +Both `FederationJCache` and `FederationCaffeineCache` implement the same unsynchronized check-then-act pattern across three methods: + +### `FederationJCache.getSubClusters` (lines 101–110) + +```java +public Map getSubClusters(boolean filterInactiveSubClusters) + throws YarnException { + final String cacheKey = buildCacheKey(className, GET_SUBCLUSTERS_CACHEID, + Boolean.toString(filterInactiveSubClusters)); + CacheRequest cacheRequest = cache.get(cacheKey); // (1) all N threads: miss + if (cacheRequest == null) { + cacheRequest = buildGetSubClustersCacheRequest(className, filterInactiveSubClusters); // (2) N×stateStore.getSubClusters() → ZooKeeper/SQL + cache.put(cacheKey, cacheRequest); // (3) N redundant puts + } + return buildSubClusterInfoMap(cacheRequest); +} +``` + +The same pattern exists verbatim in: +- `FederationJCache.getPoliciesConfigurations` (lines 113–123) — calls `stateStore.getPoliciesConfigurations()` +- `FederationJCache.getApplicationHomeSubCluster` (lines 125–138) — calls `stateStore.getApplicationHomeSubCluster()` +- All three methods duplicated identically in `FederationCaffeineCache.java` + +### The state store calls are network calls + +`buildGetSubClustersCacheRequest` calls `stateStore.getSubClusters(request)` which in production is backed by: +- `ZookeeperFederationStateStore` — ZooKeeper read (10–50ms each) +- `SQLFederationStateStore` — SQL database query (5–100ms each) + +When TTL expires (default: `yarn.federation.cache-ttl.secs` = 60s) under load, N threads all miss simultaneously and all fire the state store call. + +### Default configuration makes this the production path + +`YarnConfiguration.java`: +```java +public static final String FEDERATION_CACHE_CLASS_DEFAULT = + "org.apache.hadoop.yarn.server.federation.cache.FederationJCache"; +``` + +`FederationJCache` is used unless explicitly overridden. `FederationCaffeineCache` was added as an alternative but carries the exact same defect. + +## Impact + +YARN Router in a large federation (50+ sub-clusters, 10,000+ applications) issues these calls on every resource allocation request. At cache TTL expiry under high load: +- N concurrent Router threads all miss `getSubClusters` +- N ZooKeeper reads fire simultaneously +- ZooKeeper watch limit or connection pool exhausted +- Allocation requests time out +- Entire allocation path stalls until cache is populated + +## Fix + +### Option 1: Use `computeIfAbsent` with Caffeine LoadingCache (preferred for `FederationCaffeineCache`) + +```java +// FederationCaffeineCache — replace manual check-then-act with LoadingCache +private LoadingCache cache; + +// In initCache(): +this.cache = Caffeine.newBuilder() + .maximumSize(cacheEntityNums) + .expireAfterWrite(cacheTimeToLive, TimeUnit.SECONDS) + .build(key -> loadFromStateStore(key)); // loader called exactly once per key per miss + +// In getSubClusters(): +public Map getSubClusters(boolean filterInactiveSubClusters) + throws YarnException { + final String cacheKey = buildCacheKey(className, GET_SUBCLUSTERS_CACHEID, + Boolean.toString(filterInactiveSubClusters)); + return buildSubClusterInfoMap(cache.get(cacheKey)); // LoadingCache serializes misses per key +} +``` + +### Option 2: Synchronized block around miss path (for `FederationJCache`) + +```java +public Map getSubClusters(boolean filterInactiveSubClusters) + throws YarnException { + final String cacheKey = buildCacheKey(className, GET_SUBCLUSTERS_CACHEID, + Boolean.toString(filterInactiveSubClusters)); + CacheRequest cacheRequest = cache.get(cacheKey); + if (cacheRequest == null) { + synchronized (this) { + cacheRequest = cache.get(cacheKey); // double-checked locking + if (cacheRequest == null) { + cacheRequest = buildGetSubClustersCacheRequest(className, filterInactiveSubClusters); + cache.put(cacheKey, cacheRequest); + } + } + } + return buildSubClusterInfoMap(cacheRequest); +} +``` + +Apply the same fix to `getPoliciesConfigurations` and `getApplicationHomeSubCluster` in both `FederationJCache` and `FederationCaffeineCache`. diff --git a/defects/hibernate-orm/patch/hibernate-orm-MOADX-0001-query-interpretation-cache-stampede.md b/defects/hibernate-orm/patch/hibernate-orm-MOADX-0001-query-interpretation-cache-stampede.md new file mode 100644 index 000000000..16dd742b2 --- /dev/null +++ b/defects/hibernate-orm/patch/hibernate-orm-MOADX-0001-query-interpretation-cache-stampede.md @@ -0,0 +1,156 @@ +# UNDF: (pending) +# hibernate-orm-MOADX-0001: QueryInterpretationCacheStandardImpl — cache stampede O(N) concurrent plan builds on cold start + +## MOAD-0006 Candidate — The Thundering Herd + +| Field | Value | +|-------|-------| +| ID | hibernate-orm-MOADX-0001 | +| Severity | HIGH | +| Ecosystem | hibernate-orm | +| File | `hibernate-core/src/main/java/org/hibernate/query/internal/QueryInterpretationCacheStandardImpl.java` | +| Lines | 81–103 (`resolveSelectQueryPlan`), 115–149 (`resolveHqlInterpretation`) | +| Pattern | get → null check → compute → put (no lock, no computeIfAbsent) | +| Trigger | N concurrent requests with same uncached query → N concurrent expensive plan builds | +| Hot path | Per-query execution path: every `session.createQuery()` / `session.createMutationQuery()` call | +| Compute cost | HQL → SQM AST translation + SQL query plan building (type resolution, join analysis, SQL generation strategy) | + +## Defect + +`QueryInterpretationCacheStandardImpl` is Hibernate ORM's query plan cache, used on every HQL/JPQL query execution. Two methods exhibit the classic check-then-act stampede pattern on the underlying `InternalCache` (`BoundedConcurrentHashMap`): + +### `resolveSelectQueryPlan` (lines 81–103) + +```java +@Override +public SelectQueryPlan resolveSelectQueryPlan( + K key, + Function> creator) { + @SuppressWarnings("unchecked") + final var cached = (SelectQueryPlan) queryPlanCache.get( key ); // 1. read + if ( cached != null ) { + return cached; // 2. hit + } + + final var plan = creator.apply( key ); // 3. expensive: SQL plan build + queryPlanCache.put( key.prepareForStore(), plan ); // 4. write + return plan; +} +``` + +`creator.apply(key)` calls `buildSelectQueryPlan()` → `QuerySplitter.split()` + `buildConcreteQueryPlan()` → `new ConcreteSqmSelectQueryPlan(...)`. This involves SQM polymorphism expansion, entity graph resolution, fetch strategy analysis, and SQL generation strategy computation. + +### `resolveHqlInterpretation` (lines 115–149) + +```java +@Override +public HqlInterpretation resolveHqlInterpretation( + String queryString, + Class expectedResultType, + HqlTranslator translator) { + ... + final var existing = hqlInterpretationCache.get( cacheKey ); // 1. read + if ( existing != null ) { + return (HqlInterpretation) existing; // 2. hit + } + // optional second lookup for query-only key + final var hqlInterpretation = + createHqlInterpretation( queryString, expectedResultType, translator, statistics ); // 3. expensive + hqlInterpretationCache.put( cacheKey, hqlInterpretation ); // 4. write + return hqlInterpretation; +} +``` + +`createHqlInterpretation()` calls `translator.translate(queryString, expectedResultType)` — full HQL lexing, parsing, AST construction, semantic analysis (type binding, relationship traversal, parameter extraction). + +### Inconsistency + +`resolveNativeQueryParameters` (line 192) in the *same class* correctly uses `computeIfAbsent`: + +```java +return nativeQueryParamCache.computeIfAbsent( queryString, creator ); +``` + +This confirms the fix was known but not applied consistently. + +## Stampede Scenario + +1. Application starts and first `N` concurrent HTTP requests all execute the same JPQL query (e.g. `SELECT u FROM User u WHERE u.id = :id`). +2. All N threads call `resolveHqlInterpretation` simultaneously. +3. All N threads see a cache miss (step 1) — the cache is empty. +4. All N threads enter `createHqlInterpretation` simultaneously. +5. All N threads independently: lex and parse the HQL string, build an SQM AST, resolve entity types, extract parameters. +6. All N writes race to `put` — result is the same but work was done N times. +7. Under moderate load (N=50 threads, ~20 query types), 1000 duplicate plan builds execute before the cache warms. + +Same cascade applies after Hibernate's `queryPlanCacheMaxSize` evicts a plan (default 2048) — a popular query that gets evicted causes a burst of recomputes. + +## Fix + +Replace the get-null-compute-put triple with a single `computeIfAbsent` call, consistent with what `resolveNativeQueryParameters` already does. + +### `resolveSelectQueryPlan` + +```java +@Override +public SelectQueryPlan resolveSelectQueryPlan( + K key, + Function> creator) { + LOG.tracef( "Resolving cached query plan for [%s]", key ); + final var statistics = getStatistics(); + final boolean statisticsEnabled = statistics.isStatisticsEnabled(); + + @SuppressWarnings("unchecked") + final var plan = (SelectQueryPlan) queryPlanCache.computeIfAbsent( + key, + k -> { + if ( statisticsEnabled ) { + statistics.queryPlanCacheMiss( k.getQueryString() ); + } + return creator.apply( (K) k ); + }); + + if ( statisticsEnabled && plan != null ) { + // hit was recorded if computeIfAbsent returned without calling the function + // (we cannot distinguish hit from miss here without a flag, but misses are counted inside) + } + return plan; +} +``` + +Or simpler — use the existing `computeIfAbsent` interface on `InternalCache`: + +```java +final var plan = (SelectQueryPlan) queryPlanCache.computeIfAbsent( + key, + k -> creator.apply( (K) k ) ); +``` + +### `resolveHqlInterpretation` + +```java +@Override +public HqlInterpretation resolveHqlInterpretation( + String queryString, + Class expectedResultType, + HqlTranslator translator) { + final Object cacheKey = expectedResultType != null + ? new HqlInterpretationCacheKey( queryString, expectedResultType ) + : queryString; + + @SuppressWarnings("unchecked") + final var result = (HqlInterpretation) hqlInterpretationCache.computeIfAbsent( + cacheKey, + k -> createHqlInterpretation( queryString, expectedResultType, translator, getStatistics() ) ); + return result; +} +``` + +Note: `InternalCache.computeIfAbsent` is already defined in the interface (line in `InternalCache.java`) and implemented in `LegacyInternalCacheImplementation` via `BoundedConcurrentHashMap.computeIfAbsent`. The infrastructure is in place — only the call sites need updating. + +## Severity Rationale + +- **HIGH**: Called on every query execution, not just startup. Cache misses recur after eviction. +- Compute cost: HQL parsing + type resolution is CPU-intensive and involves synchronization inside the HQL translator. +- At N=100 concurrent requests on a cold start (k8s pod restart, cache eviction), 100 redundant plan builds stall the thread pool simultaneously — classic thundering herd causing cascading latency. +- The fix requires changing two call sites to use the already-present `computeIfAbsent` API. diff --git a/defects/kafka/patch/kafka-MOADX-0001-connect-cached-connectors-stampede.md b/defects/kafka/patch/kafka-MOADX-0001-connect-cached-connectors-stampede.md new file mode 100644 index 000000000..d418b5d36 --- /dev/null +++ b/defects/kafka/patch/kafka-MOADX-0001-connect-cached-connectors-stampede.md @@ -0,0 +1,93 @@ +# UNDF: (pending) +# kafka-MOADX-0001: CachedConnectors.lookup — TOCTOU stampede on connector class loading + +## MOAD-0006 Candidate — The Thundering Herd + +| Field | Value | +|-------|-------| +| ID | kafka-MOADX-0001 | +| Severity | MEDIUM | +| Ecosystem | Apache Kafka Connect | +| File | `connect/runtime/src/main/java/org/apache/kafka/connect/runtime/CachedConnectors.java` lines 57–73 | +| Cache type | Connector class instance cache (connector name + version → Connector object) | +| Pattern | containsKey+get (non-atomic) → null check → plugins.newConnector() (classloader scan + instantiation) → put (unsynchronized) | +| Trigger | N concurrent connector-start requests for the same connector type all fail the containsKey check → N classloader scans and instantiations | +| Hot path | `WorkerConnector.start()` and `Worker.startConnector()` — called once per connector instance startup | + +## Defect + +`CachedConnectors.lookup` performs a check-then-act that is not atomic, allowing multiple threads to all miss the cache for the same (connectorName, version) key simultaneously: + +```java +private Connector lookup(String connectorName, VersionRange range) { + String version = range == null ? LATEST_VERSION : range.toString(); + // TOCTOU: containsKey + get is not atomic — another thread can insert between these two calls + if (connectors.containsKey(connectorName) && connectors.get(connectorName).containsKey(version)) { + return connectors.get(connectorName).get(version); // (1) all N threads: miss (concurrent startup) + } + + try { + Connector connector = plugins.newConnector(connectorName, range); // (2) N×classloader scan + instantiation + connectors.computeIfAbsent(connectorName, k -> new ConcurrentHashMap<>()).put(version, connector); // (3) N puts + return connector; + } catch (VersionedPluginLoadingException e) { + invalidVersions.computeIfAbsent(connectorName, k -> new ConcurrentHashMap<>()).put(version, e); + throw e; + } catch (Exception e) { + invalidConnectors.put(connectorName, e); + throw e; + } +} +``` + +### What `plugins.newConnector` does + +`plugins.newConnector(connectorName, range)` calls `pluginLoader(classOrAlias, range, sourceLoader).loadClass(classOrAlias)` which: +1. Scans all plugin sources for the connector class (classpath scan if using reflective discovery) +2. Creates an isolated classloader (`DelegatingClassLoader`) for the plugin +3. Instantiates the connector via reflection + +This is CPU-intensive (classpath scan can take 10–500ms depending on plugin count) and involves `synchronized` blocks inside the JVM class loading machinery. Under concurrent load, multiple threads competing for the same class create contention inside `ClassLoader.loadClass`. + +### Scenario + +A Kafka Connect cluster running in distributed mode adds a new connector with 100 tasks. All 100 task startup requests arrive at the `Worker` simultaneously. Each calls `getConnector(connectorName, null)` → `lookup()`. All 100 threads fail the `containsKey` check and all fire `plugins.newConnector()`. The JVM class loading lock then serializes them, but the outer scan work is duplicated 100×. + +### Note on outer `connectors` map + +`connectors` is a `ConcurrentHashMap>` but the inner map is a freshly created `ConcurrentHashMap` added via `computeIfAbsent`. The race is: thread A checks `containsKey`, thread B also checks `containsKey` (both miss), both call `newConnector`, both try to `computeIfAbsent` the inner map and `put`. Both complete — correctness is preserved but the redundant class loading is wasteful. + +## Impact + +- On startup with many connector tasks: N redundant classloader scans and instantiations +- JVM class loading lock contention under the concurrent instantiation attempts +- Connector plugins that have expensive `init()` or static initializers run N times +- MEDIUM severity because this is not a per-request hot path — it happens at connector start only, not per-message + +## Fix + +Use `computeIfAbsent` directly to make the check-and-insert atomic: + +```java +private Connector lookup(String connectorName, VersionRange range) { + String version = range == null ? LATEST_VERSION : range.toString(); + // Atomic: computeIfAbsent ensures loader is called exactly once per (name, version) pair + Map versionMap = connectors.computeIfAbsent( + connectorName, k -> new ConcurrentHashMap<>()); + return versionMap.computeIfAbsent(version, v -> { + try { + return plugins.newConnector(connectorName, range); + } catch (VersionedPluginLoadingException e) { + invalidVersions.computeIfAbsent(connectorName, k -> new ConcurrentHashMap<>()).put(version, e); + throw e; + } catch (Exception e) { + invalidConnectors.put(connectorName, e); + throw e; + } + }); +} +``` + +Note: `computeIfAbsent` on `ConcurrentHashMap` guarantees the mapping function is called at most once per key per miss (per the Java 8+ specification). This eliminates both the redundant instantiation and the TOCTOU window. + +The error-caching paths (`invalidConnectors`, `invalidVersions`) should similarly be guarded, but errors are less likely to be concurrent. diff --git a/defects/spring/patch/spring-MOADX-0001-fallback-cache-operation-source-stampede.md b/defects/spring/patch/spring-MOADX-0001-fallback-cache-operation-source-stampede.md new file mode 100644 index 000000000..9b0c2f883 --- /dev/null +++ b/defects/spring/patch/spring-MOADX-0001-fallback-cache-operation-source-stampede.md @@ -0,0 +1,138 @@ +# UNDF: (pending) +# spring-MOADX-0001: AbstractFallbackCacheOperationSource — cache stampede O(N) concurrent annotation scans on cold start + +## MOAD-0006 Candidate — The Thundering Herd + +| Field | Value | +|-------|-------| +| ID | spring-MOADX-0001 | +| Severity | MEDIUM | +| Ecosystem | spring-framework | +| File 1 | `spring-context/src/main/java/org/springframework/cache/interceptor/AbstractFallbackCacheOperationSource.java` | +| File 2 | `spring-context-support/src/main/java/org/springframework/cache/jcache/interceptor/AbstractFallbackJCacheOperationSource.java` | +| Lines | 95–121 (standard), 67–91 (JCache) | +| Pattern | ConcurrentHashMap.get → null check → reflection scan → ConcurrentHashMap.put | +| Trigger | N concurrent requests to same `@Cacheable`-annotated method before cache warms → N concurrent annotation reflection scans | +| Hot path | Every `@Cacheable` / `@CacheResult` method invocation before the operation metadata is cached | +| Compute cost | Java reflection: `AopUtils.getMostSpecificMethod`, method/class annotation traversal, proxy unwrapping | + +## Defect + +`AbstractFallbackCacheOperationSource` is the core metadata resolver for Spring's `@Cacheable`, `@CachePut`, and `@CacheEvict` annotations. It caches the resolved `CacheOperation` collection in a `ConcurrentHashMap`, but uses a non-atomic get-null-compute-put pattern: + +```java +// AbstractFallbackCacheOperationSource.java:95–121 +private @Nullable Collection getCacheOperations( + Method method, @Nullable Class targetClass, boolean cacheNull) { + + if (ReflectionUtils.isObjectMethod(method)) { + return null; + } + + Object cacheKey = getCacheKey(method, targetClass); + Collection cached = this.operationCache.get(cacheKey); // 1. read + + if (cached != null) { + return (cached != NULL_CACHING_MARKER ? cached : null); // 2. hit + } + else { + Collection cacheOps = computeCacheOperations(method, targetClass); // 3. compute + if (cacheOps != null) { + this.operationCache.put(cacheKey, cacheOps); // 4. write + } + else if (cacheNull) { + this.operationCache.put(cacheKey, NULL_CACHING_MARKER); + } + return cacheOps; + } +} +``` + +`computeCacheOperations` resolves the most-specific method (proxy unwrapping), then walks the method and declaring class for cache annotations — up to 4 reflection calls: + +```java +private @Nullable Collection computeCacheOperations(Method method, @Nullable Class targetClass) { + Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); // proxy unwrap + class hierarchy + Collection opDef = findCacheOperations(specificMethod); // method annotation scan + if (opDef != null) return opDef; + opDef = findCacheOperations(specificMethod.getDeclaringClass()); // class annotation scan + if (opDef != null && ClassUtils.isUserLevelMethod(method)) return opDef; + if (specificMethod != method) { + opDef = findCacheOperations(method); // original method scan + if (opDef != null) return opDef; + opDef = findCacheOperations(method.getDeclaringClass()); // original class scan + if (opDef != null && ClassUtils.isUserLevelMethod(method)) return opDef; + } + return null; +} +``` + +The identical pattern exists in `AbstractFallbackJCacheOperationSource.getCacheOperation` for `@CacheResult`/`@CachePut`/`@CacheRemove` annotations (lines 67–91). + +## Stampede Scenario + +1. Application receives `N` concurrent requests to the same `@Cacheable`-annotated method (e.g., a REST endpoint called on startup health check or during load test ramp-up). +2. The `operationCache` is empty (cold start, or first invocation after context refresh). +3. All N threads call `getCacheOperations` simultaneously. +4. All N threads get a cache miss (step 1). +5. All N threads independently call `computeCacheOperations`: proxy unwrapping, annotation scanning. +6. All N threads race to `put` the same result — identical values, redundant work. + +In a large Spring application with many `@Cacheable` beans, concurrent startup traffic forces redundant annotation scans across hundreds of method-class pairs simultaneously. This delays initial response times and causes contention in the reflection infrastructure. + +## Fix + +Replace get-null-compute-put with `computeIfAbsent`. The `ConcurrentHashMap` already supports this atomically: + +### Standard cache annotation source + +```java +// AbstractFallbackCacheOperationSource.java +private @Nullable Collection getCacheOperations( + Method method, @Nullable Class targetClass, boolean cacheNull) { + + if (ReflectionUtils.isObjectMethod(method)) { + return null; + } + + Object cacheKey = getCacheKey(method, targetClass); + // computeIfAbsent is atomic: only one thread computes, others wait for the result + Object cached = this.operationCache.computeIfAbsent(cacheKey, k -> { + Collection ops = computeCacheOperations(method, targetClass); + return (ops != null) ? ops : (cacheNull ? NULL_CACHING_MARKER : null); + }); + + if (cached == null) { + return null; // cacheNull was false and no operation found + } + return (cached != NULL_CACHING_MARKER ? (Collection) cached : null); +} +``` + +Note: `ConcurrentHashMap.computeIfAbsent` does not accept null return values; the `NULL_CACHING_MARKER` sentinel handles the "no annotation" case correctly. + +### JCache annotation source + +Same fix applies to `AbstractFallbackJCacheOperationSource.getCacheOperation`: + +```java +private @Nullable JCacheOperation getCacheOperation(Method method, @Nullable Class targetClass, boolean cacheNull) { + if (ReflectionUtils.isObjectMethod(method)) { + return null; + } + MethodClassKey cacheKey = new MethodClassKey(method, targetClass); + Object cached = this.operationCache.computeIfAbsent(cacheKey, k -> { + JCacheOperation op = computeCacheOperation(method, targetClass); + return (op != null) ? op : (cacheNull ? NULL_CACHING_MARKER : null); + }); + if (cached == null) return null; + return (cached != NULL_CACHING_MARKER ? (JCacheOperation) cached : null); +} +``` + +## Severity Rationale + +- **MEDIUM**: The compute cost (annotation reflection) is lower than I/O or SQL plan building, but reflection is synchronized on the JVM class object and can serialize under high parallelism. +- Triggered on every cold start or after Spring context refresh — affects all `@Cacheable`-heavy applications. +- At N=200 concurrent startup requests across 50 annotated methods, 10,000 redundant annotation scans execute before the cache warms. Each scan acquires internal JVM locks. +- The fix is a one-line change per method — `computeIfAbsent` is already present on the `ConcurrentHashMap` backing the `operationCache`. diff --git a/defects/traefik/patch/traefik-MOADX-0001-cname-flatten-cache-stampede.md b/defects/traefik/patch/traefik-MOADX-0001-cname-flatten-cache-stampede.md new file mode 100644 index 000000000..80b805447 --- /dev/null +++ b/defects/traefik/patch/traefik-MOADX-0001-cname-flatten-cache-stampede.md @@ -0,0 +1,114 @@ +# UNDF: (pending) +# traefik-MOADX-0001: CNAMEFlatten — cache stampede + data race O(N) concurrent DNS lookups + +## MOAD-0006 Candidate — The Thundering Herd + +| Field | Value | +|-------|-------| +| ID | traefik-MOADX-0001 | +| Severity | HIGH | +| Ecosystem | Traefik | +| Language | Go | +| File | `pkg/middlewares/requestdecorator/hostresolver.go` | +| Lines | 37–75 | +| Pattern | get → miss → expensive DNS lookup → set (no singleflight, no mutex) | +| Trigger | N concurrent goroutines all hit same hostname with cold/expired cache | +| Compute cost | Full CNAME-chain DNS resolution via `cnameResolve()` with 30-second timeout per DNS server | + +## Defect + +`CNAMEFlatten` is called by `RequestDecorator.ServeHTTP` on every inbound HTTP request in the entrypoint's handler chain. The `Resolver` struct is a single instance shared across all goroutines for that entrypoint (created once in `NewTCPEntryPoint`, passed to `requestdecorator.New`). + +**Race 1 — lazy cache init data race:** + +```go +// pkg/middlewares/requestdecorator/hostresolver.go:38-39 +func (hr *Resolver) CNAMEFlatten(ctx context.Context, host string) string { + if hr.cache == nil { + hr.cache = cache.New(30*time.Minute, 5*time.Minute) // DATA RACE + } + // ... +} +``` + +Multiple goroutines read `hr.cache == nil` simultaneously, then all write `hr.cache = ...`. Go's race detector flags this immediately. The `*cache.Cache` pointer is written without any synchronization. + +**Race 2 — cache stampede on miss:** + +```go +// pkg/middlewares/requestdecorator/hostresolver.go:50-68 +value, found := hr.cache.Get(host) // thread-safe individually +if found { + return value.(string) +} + +// NO LOCK OR SINGLEFLIGHT AROUND THIS BLOCK: +for depth := range hr.ResolvDepth { + resolv, err := cnameResolve(ctx, request, hr.ResolvConfig) // expensive DNS call + // ... +} + +hr.cache.Set(host, result, cacheDuration) // thread-safe individually +``` + +`go-cache`'s `Get` and `Set` are individually mutex-protected, but the check-then-compute-then-set sequence is not atomic. When N goroutines all request the same hostname simultaneously on a cold or expired cache entry, all N pass the `found == false` check and all N execute `cnameResolve()` in parallel — each doing up to `ResolvDepth` DNS round-trips (each with 30-second timeouts). + +**Grower signal:** Works fine at low traffic. Under a traffic spike to a new backend hostname, or when the 30-minute TTL expires on a popular hostname, all concurrent requests trigger DNS lookups simultaneously. With `ResolvDepth=5` (default) and N=1000 concurrent goroutines, this is 5000 concurrent DNS UDP/TCP connections. + +## Fix + +**Option A — `singleflight.Group` (preferred, matches Traefik's existing pattern):** + +```go +import "golang.org/x/sync/singleflight" + +type Resolver struct { + CnameFlattening bool + ResolvConfig string + ResolvDepth int + cache *cache.Cache + cacheOnce sync.Once + group singleflight.Group +} + +func (hr *Resolver) CNAMEFlatten(ctx context.Context, host string) string { + hr.cacheOnce.Do(func() { + hr.cache = cache.New(30*time.Minute, 5*time.Minute) + }) + + if value, found := hr.cache.Get(host); found { + return value.(string) + } + + result, _, _ := hr.group.Do(host, func() (any, error) { + // Only one goroutine resolves; others wait and share the result. + res := host + req := host + cacheDuration := 0 * time.Second + for depth := range hr.ResolvDepth { + resolv, err := cnameResolve(ctx, req, hr.ResolvConfig) + if err != nil || resolv == nil { + break + } + res = resolv.Record + if depth == 0 { + cacheDuration = resolv.TTL + } + req = resolv.Record + } + hr.cache.Set(host, res, cacheDuration) + return res, nil + }) + return result.(string) +} +``` + +**Option B — `sync.Once` for init + `sync.Map` as cache:** + +Replace `*cache.Cache` with `sync.Map` and use a per-key `singleflight.Group`. Traefik already uses `singleflight` in `basic_auth.go` (line 118) and `healthcheck.go` — the pattern is established in the codebase. + +## Impact + +- **Data race:** Go runtime will panic under `-race` flag; undefined behavior in production (cache pointer torn write). +- **Stampede:** N×ResolvDepth DNS connections on cache miss. At N=1000 goroutines with ResolvDepth=5, a single cache expiry produces 5000 DNS queries from one Traefik instance. DNS resolver exhaustion, connection table blowup, upstream SERVFAIL cascade. +- **Fix speedup:** After fix, cache miss costs 1 DNS lookup regardless of concurrent request count.