russell@unturf.com
25c2bafdee
New UNDF assignments (693→720): elixir-0002 → UNDF-2026-000000698 (typespec used_type_pairs O(T²)) r-source-0002 → UNDF-2026-000000711 (.walkClassGraph match dedup O(S²)) ruby-0003 → UNDF-2026-000000712 (RubyGems dependent_gems O(N²×D)) victoria-metrics-0002 → UNDF-2026-000000717 (MetricName tag-filter O(T×I)) Total: 720 UNDF assigned
118 lines
4.9 KiB
Markdown
118 lines
4.9 KiB
Markdown
# UNDF: UNDF-2026-000000706
|
|
# netty-0001: DnsResolveContext.finalResult ArrayList dedup O(R²)
|
|
|
|
## Classification
|
|
- **CWE**: CWE-407 (Inefficient Algorithmic Complexity)
|
|
- **Severity**: MEDIUM
|
|
- **Component**: Netty — `resolver-dns`
|
|
- **File**: `resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java`
|
|
- **Method**: `onResponseAddRecord` (inner, around line 914)
|
|
- **Complexity**: O(R²) where R = total DNS records accumulated across all nameserver responses
|
|
|
|
## Description
|
|
|
|
`DnsResolveContext` accumulates resolved records into `finalResult`, an `ArrayList<T>`.
|
|
Before adding each new record, it checks for duplicates via `finalResult.contains(converted)`.
|
|
`ArrayList.contains` is an O(N) linear scan, making the dedup loop O(R²) over all R records
|
|
collected during a hostname resolution.
|
|
|
|
The code contains a self-aware comment that admits a `LinkedHashSet` would be better, but
|
|
incorrectly dismisses it:
|
|
|
|
```java
|
|
// While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an
|
|
// ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay
|
|
// for the extra memory copy and allocations in this cases later on.
|
|
```
|
|
|
|
This reasoning is flawed: a `LinkedHashSet` avoids the copy because `toArray()` can be
|
|
called once at the end; duplicates are the normal case for multi-server failover (each
|
|
server returns the same A records), not rare. With 5 nameservers each returning 10 A
|
|
records, `contains()` is called 40+ times scanning a list growing from 1 to 10 elements —
|
|
O(40) vs O(10) for a set. With CNAME chains and search domain retries that accumulate
|
|
records from multiple passes, the list can reach 20-50 entries, making O(R²) more visible.
|
|
|
|
The `DnsAddressResolveContext` subclass sets `isDuplicateAllowed()` to `false`, meaning
|
|
this dedup path is always executed for address resolution.
|
|
|
|
## Defect Code
|
|
|
|
```java
|
|
// resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java lines ~906-916
|
|
if (finalResult == null) {
|
|
finalResult = new ArrayList<T>(8);
|
|
finalResult.add(converted);
|
|
} else if (isDuplicateAllowed() || !finalResult.contains(converted)) { // O(N) scan
|
|
finalResult.add(converted);
|
|
} else {
|
|
shouldRelease = true;
|
|
}
|
|
```
|
|
|
|
## Fix
|
|
|
|
Replace `ArrayList` with `LinkedHashSet` for dedup tracking, then convert to list at
|
|
`finishResolve()` when results are returned. This maintains insertion order while giving
|
|
O(1) dedup.
|
|
|
|
```java
|
|
// Change field declaration
|
|
- private List<T> finalResult;
|
|
+ private Set<T> finalResult; // LinkedHashSet maintains insertion order, O(1) contains
|
|
|
|
// Change allocation site
|
|
- finalResult = new ArrayList<T>(8);
|
|
+ finalResult = new LinkedHashSet<T>(8);
|
|
|
|
// finishResolve already calls filterResults which can consume any Collection;
|
|
// the List<T> result = filterResults(finalResult) call works with a Set input.
|
|
```
|
|
|
|
## Patch
|
|
|
|
```diff
|
|
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
|
|
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
|
|
@@ -116,7 +116,8 @@ abstract class DnsResolveContext<T> {
|
|
private int allowedQueries;
|
|
private boolean triedCNAME;
|
|
private boolean completeEarly;
|
|
- private List<T> finalResult;
|
|
+ // LinkedHashSet gives O(1) dedup while preserving insertion order
|
|
+ private Set<T> finalResult;
|
|
|
|
@@ -909,11 +909,8 @@ abstract class DnsResolveContext<T> {
|
|
if (!promise.isDone()) {
|
|
- // We want to ensure we do not have duplicates in finalResult as this may be unexpected.
|
|
- //
|
|
- // While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an
|
|
- // ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay
|
|
- // for the extra memory copy and allocations in this cases later on.
|
|
if (finalResult == null) {
|
|
- finalResult = new ArrayList<T>(8);
|
|
- finalResult.add(converted);
|
|
- } else if (isDuplicateAllowed() || !finalResult.contains(converted)) {
|
|
+ finalResult = new LinkedHashSet<T>(8);
|
|
+ }
|
|
+ if (isDuplicateAllowed() || finalResult.add(converted)) {
|
|
finalResult.add(converted);
|
|
} else {
|
|
shouldRelease = true;
|
|
```
|
|
|
|
## Complexity Comparison
|
|
|
|
| N records (total across all responses) | Old (ArrayList.contains) | New (LinkedHashSet.add) |
|
|
|----------------------------------------|--------------------------|-------------------------|
|
|
| 10 | 45 ops | 10 ops |
|
|
| 20 | 190 ops | 20 ops |
|
|
| 50 | 1,225 ops | 50 ops |
|
|
|
|
Ratio at N=50: **24.5x**
|
|
|
|
## Hot Path
|
|
|
|
Called on every DNS record during `DnsResolveContext` record accumulation — which happens
|
|
once per DNS address resolution. For services doing frequent hostlookups (microservice
|
|
discovery, gRPC name resolution, load-balancer refresh), this executes thousands of times
|
|
per second.
|