Skip to content

dns: preserve reentrant c-ares query after qid reuse - #46880

Open
fvallenilla wants to merge 2 commits into
envoyproxy:mainfrom
fvallenilla:fv/cares-qid-ownership-guard-upstream
Open

dns: preserve reentrant c-ares query after qid reuse#46880
fvallenilla wants to merge 2 commits into
envoyproxy:mainfrom
fvallenilla:fv/cares-qid-ownership-guard-upstream

Conversation

@fvallenilla

@fvallenilla fvallenilla commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Commit Message:

dns: preserve reentrant c-ares query after QID reuse

A July security fix for c-ares (CVE-2026-33630) made query teardown run twice: once before the completion callback, once after. Between those two teardowns, the callback can enqueue a new query with the same 16-bit transaction ID. The second teardown then deletes the map entry by key — evicting the new query if there was a collision.

As a result: c-ares discards the responses for the new query, does not call its callback, and Envoy can permanently stop refreshing the affected DNS cluster.

The change in this PR is to only remove a completing query's QID entry when the table still points to that query. A replacement query that has taken ownership of the same QID remains registered and completes normally.

Before and after diagrams

Before patch

sequenceDiagram
    participant E as Envoy AUTO resolver
    participant V6 as Completing AAAA query
    participant Q as queries_by_qid
    participant V4 as Reentrant A query

    E->>V6: Resolve AAAA for IPv4-only hostname
    V6->>Q: QID 42 points to AAAA query
    V6-->>E: Return NODATA in callback
    E->>V4: Start A fallback inline
    V4->>Q: Randomly select the same QID 42 for A query
    E-->>V6: Callback returns
    V6->>Q: Remove QID 42 unconditionally
    Note over V4,Q: A query loses its table entry
    Note over E: Refresh loop stalls
Loading

After patch

sequenceDiagram
    participant E as Envoy AUTO resolver
    participant V6 as Completing AAAA query
    participant Q as queries_by_qid
    participant V4 as Reentrant A query

    E->>V6: Resolve AAAA for IPv4-only hostname
    V6->>Q: QID 42 points to AAAA query
    V6-->>E: Return NODATA in callback
    E->>V4: Start A fallback inline
    V4->>Q: Randomly reuse QID 42 for A query
    E-->>V6: Callback returns
    V6->>Q: Remove only if QID 42 still points to AAAA query
    Q-->>V6: QID 42 now points to A query
    V6-->>Q: Keep QID 42
    Note over V4,Q: A query retains its table entry
    V4-->>E: Return IPv4 addresses
    E->>E: Schedule next DNS refresh
Loading

Additional Description:

This ports the guard from c-ares/c-ares#1256 while Envoy remains on c-ares 1.34.8. The failure and a minimal natural-QID reproduction are documented in #46877.

AI assistance was used to prepare this draft. The submitter will review and take ownership before marking it ready for review.

Risk Level: Low

Testing:

  • bazel build @c-ares//:ares
  • bazel run //tools/code_format:check_format -- check bazel/c-ares.patch changelogs/current/bug_fixes/dns_resolver__preserve-reused-cares-qid.rst
  • Manual end-to-end validation with the equivalent c-ares 1.34.8 patch: a single Envoy STRICT_DNS cluster at a 2 ms refresh interval hit a natural QID collision after 119,436 opportunities, processed both the collision and following normal address, and completed another 8,637 successful refreshes with zero failures.

Docs Changes: N/A

Release Notes: Added a DNS resolver bug-fix fragment.

Platform Specific Features: N/A

[Optional Runtime guard:] N/A; this is a dependency correctness fix that preserves the current query owner.

Fixes #46877

Signed-off-by: Freddy Vallenilla <fvallenilla@netflix.com>
@repokitteh-read-only

Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #46880 was opened by fvallenilla.

see: more, trace.

@fvallenilla
fvallenilla marked this pull request as ready for review August 21, 2026 23:07
@repokitteh-read-only repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Aug 21, 2026
@repokitteh-read-only

Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #46880 was ready_for_review by fvallenilla.

see: more, trace.

@phlax

phlax commented Aug 24, 2026

Copy link
Copy Markdown
Member

@fvallenilla ci failures look real

/wait

@fvallenilla

Copy link
Copy Markdown
Contributor Author

@phlax The branch is up to date now, remaining failure seems unrelated

IpVersions/Http2FrameIntegrationTest.AdjustUpstreamSettingsMaxStreams/IPv6_Oghttp2

@phlax

phlax commented Aug 24, 2026

Copy link
Copy Markdown
Member

/lgtm deps

@repokitteh-read-only repokitteh-read-only Bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 24, 2026
@phlax

phlax commented Aug 24, 2026

Copy link
Copy Markdown
Member

/retest

@phlax

phlax commented Aug 24, 2026

Copy link
Copy Markdown
Member

/assign @yanavlasov for c-ares sign off

@repokitteh-read-only

Copy link
Copy Markdown

neither of for, c-ares, sign, off can be assigned to this issue.

🐱

Caused by: a #46880 (comment) was created by @phlax.

see: more, trace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

c-ares 1.34.8 QID reuse can permanently stall Envoy AUTO DNS refresh for a cluster

3 participants