fix: bound duplicate-packet dedup against alternating-payload floods by bdraco · Pull Request #1750 · python-zeroconf/python-zeroconf

bdraco · 2026-05-20T12:41:13Z

Summary

Single-slot dedup in _process_datagram_at_time only compared against self.data; alternating two byte-distinct payloads (A, B, A, B, ...) slipped past on every packet, forcing every datagram through DNSIncoming parse and the deferred-queue scan. This adds a bounded per-listener recency window so the alternating-flood case is caught while keeping the same-packet fast path on the original code path.

Details

_process_datagram_at_time keeps the existing self.data == data single-slot check as the fast path so the steady-state same-payload hit stays a pure-C compare; on miss it consults a bounded _recent_packets: dict[bytes, float] keyed on payload bytes.
recent_packets.get(data, 0.0) paired with (now - _DUPLICATE_PACKET_SUPPRESSION_INTERVAL) < recent_time keeps the suppression compare a single C double compare under Cython; stored values are millisecond timestamps so 0.0 never collides with a real entry.
Only payloads without a QU question are cached so unicast replies still go out on every receipt; the dict path therefore does not re-check has_qu_question on hits.
State lives on each AsyncListener (one per reader socket / interface) so a duplicate seen on one interface does not suppress a legitimate QU or unicast reply that arrives on another.
_RECENT_PACKETS_MAX is added to const.py and cdef'd in _listener.pxd; eviction uses dict insertion order via next(iter(recent_packets)) so the oldest entry is dropped when the window is full.
tests/_clear_cache now also resets _recent_packets and the single-slot state; tests that clear the cache between phases get a fresh listener view so legitimate retransmits inside the suppression window are not caught by the previous phase's window.

Fixes #1724.

Test plan

SKIP_CYTHON=1 poetry run pytest tests/ --no-cov --timeout=60 -- 368 passed.
REQUIRE_CYTHON=1 poetry install --only=main,dev && poetry run pytest tests/ --no-cov --timeout=60 -- 368 passed.
New tests in tests/test_listener.py:
- test_guard_against_alternating_duplicate_packets -- pins the alternating-flood guard.
- test_recent_packets_window_is_bounded -- pins eviction of the oldest entries once the window fills.
Existing test_guard_against_duplicate_packets updated to assert bounded-window semantics where the old assertions encoded the single-slot bypass.
poetry run cython -a -3 src/zeroconf/_listener.py -- the hit-path compare on line 145 stays score-0 (pure C double compare); the dict.get on line 144 is score-15 (__Pyx_PyDict_GetItemDefault + PyFloat_AsDouble).
Local steady-state timings on the tests/benchmarks/test_listener_dedup.py shapes vs master, 200 iters x 1000 runs:
- test_dedup_hit_same_payload: 6.4 us vs 6.4 us on master (no regression; fast path preserved).
- test_alternating_payloads: 8.7 us vs 78 us on master (~9x improvement; the issue).
- test_unique_payloads: 93 us vs 75 us on master (~90 ns per packet for the bounded-window bookkeeping).

codspeed-hq · 2026-05-20T12:48:55Z

Merging this PR will improve performance by ×3.4

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 1 (👁 1) regressed benchmark
✅ 12 untouched benchmarks

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`test_alternating_payloads`	1,349.5 µs	105.8 µs	×13
👁	`test_unique_payloads`	1.4 ms	1.6 ms	-11.49%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing fix/bounded-dedup-recency-window (56bb3a8) with master (304fae6)}

codecov · 2026-05-20T12:50:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.77%. Comparing base (a096238) to head (56bb3a8).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1750   +/-   ##
=======================================
  Coverage   99.76%   99.77%           
=======================================
  Files          33       33           
  Lines        3473     3486   +13     
  Branches      482      487    +5     
=======================================
+ Hits         3465     3478   +13     
  Misses          5        5           
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bdraco · 2026-05-20T12:50:11Z

@bluetoothbot review

bluetoothbot · 2026-05-20T12:57:18Z

PR Review — fix: bound duplicate-packet dedup against alternating-payload floods

Solid fix for a real issue. The single-slot dedup bypass via alternating payloads is correctly addressed by a bounded per-listener recency dict, the sentinel choice (-1e30) defends against the small-now edge case the Copilot reviewer flagged (and is pinned by test_recent_packets_miss_with_small_now_is_not_suppressed), and the bounded-window invariant is pinned by test_recent_packets_window_is_bounded. Cython surface is updated correctly in _listener.pxd (new cdef public cython.dict _recent_packets, recent_time=double local, _RECENT_PACKETS_MAX as cython.uint). The _clear_cache test helper and the in-test manual resets in test_handlers.py are kept consistent with the new state. CodSpeed shows the targeted shape ×13 faster with a small (~12%) regression on the unique-payloads benchmark, which is the expected cost of the dict bookkeeping. Only stylistic suggestions; no blocking concerns.

🟢 Suggestions

1. Document rationale for window size (`src/zeroconf/const.py`, L36)

_RECENT_PACKETS_MAX = 16 is reasonable but the choice isn't motivated anywhere. A future maintainer (or someone investigating a regression) will want to know: why 16, not 8 or 64? Worth a one-line comment naming the trade-off — large enough to defeat the alternating-flood shape from #1724, small enough that the per-entry dict overhead × N stays bounded under hostile input. Not blocking.

_RECENT_PACKETS_MAX = 16  # Bounded recency window for per-listener dedup

Checklist

Summary

Solid fix for a real issue. The single-slot dedup bypass via alternating payloads is correctly addressed by a bounded per-listener recency dict, the sentinel choice (-1e30) defends against the small-now edge case the Copilot reviewer flagged (and is pinned by test_recent_packets_miss_with_small_now_is_not_suppressed), and the bounded-window invariant is pinned by test_recent_packets_window_is_bounded. Cython surface is updated correctly in _listener.pxd (new cdef public cython.dict _recent_packets, recent_time=double local, _RECENT_PACKETS_MAX as cython.uint). The _clear_cache test helper and the in-test manual resets in test_handlers.py are kept consistent with the new state. CodSpeed shows the targeted shape ×13 faster with a small (~12%) regression on the unique-payloads benchmark, which is the expected cost of the dict bookkeeping. Only stylistic suggestions; no blocking concerns.

Automated review by Kōan303a453

Copilot

Pull request overview

This PR hardens AsyncListener._process_datagram_at_time duplicate-packet suppression to prevent trivial bypasses using alternating payloads, by adding a small bounded recency window per listener socket/interface.

Changes:

Add per-listener _recent_packets bounded recency dict (max size _RECENT_PACKETS_MAX) to suppress duplicates even when alternating between a small set of payloads.
Update tests to validate alternating-payload suppression and bounded-window eviction, and reset the new state where tests rely on “fresh” listeners.
Introduce _RECENT_PACKETS_MAX = 16 in constants and expose it to Cython via _listener.pxd.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/zeroconf/_listener.py`	Adds bounded recency duplicate suppression before parsing and populates the window for non-QU payloads.
`src/zeroconf/_listener.pxd`	Declares `_RECENT_PACKETS_MAX` and `_recent_packets` for the Cython-compiled listener and adds `recent_time` local typing.
`src/zeroconf/const.py`	Adds `_RECENT_PACKETS_MAX` constant controlling the window size.
`tests/test_listener.py`	Updates existing duplicate suppression test expectations and adds new tests for alternating payloads + eviction behavior.
`tests/test_handlers.py`	Clears `_recent_packets` between phases where tests manually bypass suppression.
`tests/__init__.py`	Extends `_clear_cache()` to reset per-listener dedup state between test phases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bdraco · 2026-05-20T13:12:49Z

@bluetoothbot review

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Replace the single-slot `self.data` remembered last packet with a bounded recency window keyed on payload bytes; an attacker alternating two byte-distinct payloads (A, B, A, B, ...) previously slipped past dedup on every packet, forcing every datagram through DNSIncoming parse and the deferred-queue dedup scan. State is kept on each AsyncListener (one per reader socket / interface) so a duplicate seen on one interface does not suppress a legitimate QU or unicast reply that arrives on another. Only non-QU payloads are cached so QU questions still get unicast replies on every receipt. The single-slot fast path is retained ahead of the dict check so the steady-state dedup hit stays a pure-C compare and matches the prior performance on `test_dedup_hit_same_payload`. Fixes #1724

bdraco · 2026-05-20T13:21:23Z

couldn't get rid of the 10% regression but it was better than the 54% regression that bot came up with. The check isn't free but it is cheap

bdraco force-pushed the fix/bounded-dedup-recency-window branch from 6d73e18 to 642efe7 Compare May 20, 2026 12:47

bdraco mentioned this pull request May 20, 2026

fix: bound duplicate-packet dedup against alternating-payload floods #1743

Closed

4 tasks

bdraco force-pushed the fix/bounded-dedup-recency-window branch from 642efe7 to 99cdade Compare May 20, 2026 12:55

bdraco requested a review from Copilot May 20, 2026 12:58

bdraco temporarily deployed to release May 20, 2026 12:59 — with GitHub Actions Inactive

Copilot started reviewing on behalf of bdraco May 20, 2026 12:59 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread src/zeroconf/_listener.py Outdated

bdraco force-pushed the fix/bounded-dedup-recency-window branch from 99cdade to 303a453 Compare May 20, 2026 13:11

bdraco requested a review from Copilot May 20, 2026 13:12

Copilot started reviewing on behalf of bdraco May 20, 2026 13:14 View session

bdraco temporarily deployed to release May 20, 2026 13:15 — with GitHub Actions Inactive

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread src/zeroconf/_listener.py

Comment thread tests/test_listener.py

bdraco force-pushed the fix/bounded-dedup-recency-window branch from 303a453 to 56bb3a8 Compare May 20, 2026 13:20

bdraco marked this pull request as ready for review May 20, 2026 13:22

bdraco temporarily deployed to release May 20, 2026 13:24 — with GitHub Actions Inactive

bdraco merged commit 8c9d6ce into master May 20, 2026
36 checks passed

bdraco deleted the fix/bounded-dedup-recency-window branch May 20, 2026 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bound duplicate-packet dedup against alternating-payload floods#1750

fix: bound duplicate-packet dedup against alternating-payload floods#1750
bdraco merged 1 commit into
masterfrom
fix/bounded-dedup-recency-window

bdraco commented May 20, 2026

Uh oh!

codspeed-hq Bot commented May 20, 2026 •

edited

Loading

Uh oh!

codecov Bot commented May 20, 2026 •

edited

Loading

Uh oh!

bdraco commented May 20, 2026

Uh oh!

bluetoothbot commented May 20, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bdraco commented May 20, 2026

Summary

Details

Test plan

Uh oh!

codspeed-hq Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will improve performance by ×3.4

Performance Changes

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bdraco commented May 20, 2026

Uh oh!

bluetoothbot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — fix: bound duplicate-packet dedup against alternating-payload floods

🟢 Suggestions

Checklist

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq Bot commented May 20, 2026 •

edited

Loading

codecov Bot commented May 20, 2026 •

edited

Loading

bluetoothbot commented May 20, 2026 •

edited

Loading