From 760121dcb1837fdee6ac7f4e2412c0d539fbff7a Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Thu, 23 Jan 2025 00:55:17 -0500 Subject: [PATCH] Fixed #35801 -- Prevented collision of senders with non-overlapping lifetimes. As documented, the id() function can return the same value for distinct objects with non-overlapping lifetimes which can result in signals being sent to the wrong receivers if two distinct senders happen to have a colliding id() value. Since reproduction of the issue requires memory constrained circumstances where the same exact id() is reused for two senders of the same signal the test opt to simulate the collision by systematically making the same id for Sender instances. Note that we explicitly avoid keeping a strong reference to senders that cannot be weakly referenced as that would unexpectedly prevent them from being garbage collected. This means that id(sender) collisions could still occur for such objects but Django itself doesn't make use of them. Thanks Sjoerd Job Postmus for the reduced test case and Mariusz for the review. Co-authored-by: And Clover --- django/dispatch/dispatcher.py | 61 +++++++++++++++++++++++------------ tests/dispatch/tests.py | 47 ++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 22 deletions(-) diff --git a/django/dispatch/dispatcher.py b/django/dispatch/dispatcher.py index fe0e1fa599..4b962ce524 100644 --- a/django/dispatch/dispatcher.py +++ b/django/dispatch/dispatcher.py @@ -28,8 +28,10 @@ class Signal: Internal attributes: - receivers - { receiverkey (id) : weakref(receiver) } + receivers: + [((id(receiver), id(sender)), ref(receiver), ref(sender), is_async)] + sender_receivers_cache: + WeakKeyDictionary[sender, list[receiver]] """ def __init__(self, use_caching=False): @@ -108,12 +110,23 @@ class Signal: ref = weakref.WeakMethod receiver_object = receiver.__self__ receiver = ref(receiver) - weakref.finalize(receiver_object, self._remove_receiver) + weakref.finalize(receiver_object, self._flag_dead_receivers) + + # Keep a weakref to sender if possible to ensure associated receivers + # are cleared if it gets garbage collected. This ensures there is no + # id(sender) collisions for distinct senders with non-overlapping + # lifetimes. + sender_ref = None + if sender is not None: + try: + sender_ref = weakref.ref(sender, self._flag_dead_receivers) + except TypeError: + pass with self.lock: self._clear_dead_receivers() - if not any(r_key == lookup_key for r_key, _, _ in self.receivers): - self.receivers.append((lookup_key, receiver, is_async)) + if not any(r_key == lookup_key for r_key, _, _, _ in self.receivers): + self.receivers.append((lookup_key, receiver, sender_ref, is_async)) self.sender_receivers_cache.clear() def disconnect(self, receiver=None, sender=None, dispatch_uid=None): @@ -410,7 +423,10 @@ class Signal: self.receivers = [ r for r in self.receivers - if not (isinstance(r[1], weakref.ReferenceType) and r[1]() is None) + if ( + not (isinstance(r[1], weakref.ReferenceType) and r[1]() is None) + and not (r[2] is not None and r[2]() is None) + ) ] def _live_receivers(self, sender): @@ -432,9 +448,14 @@ class Signal: self._clear_dead_receivers() senderkey = _make_id(sender) receivers = [] - for (_receiverkey, r_senderkey), receiver, is_async in self.receivers: + for ( + (_receiverkey, r_senderkey), + receiver, + sender_ref, + is_async, + ) in self.receivers: if r_senderkey == NONE_ID or r_senderkey == senderkey: - receivers.append((receiver, is_async)) + receivers.append((receiver, sender_ref, is_async)) if self.use_caching: if not receivers: self.sender_receivers_cache[sender] = NO_RECEIVERS @@ -443,27 +464,25 @@ class Signal: self.sender_receivers_cache[sender] = receivers non_weak_sync_receivers = [] non_weak_async_receivers = [] - for receiver, is_async in receivers: + for receiver, sender_ref, is_async in receivers: + # Skip if the receiver/sender is a dead weakref if isinstance(receiver, weakref.ReferenceType): - # Dereference the weak reference. receiver = receiver() - if receiver is not None: - if is_async: - non_weak_async_receivers.append(receiver) - else: - non_weak_sync_receivers.append(receiver) + if receiver is None: + continue + if sender_ref is not None and sender_ref() is None: + continue + if is_async: + non_weak_async_receivers.append(receiver) else: - if is_async: - non_weak_async_receivers.append(receiver) - else: - non_weak_sync_receivers.append(receiver) + non_weak_sync_receivers.append(receiver) return non_weak_sync_receivers, non_weak_async_receivers - def _remove_receiver(self, receiver=None): + def _flag_dead_receivers(self, reference=None): # Mark that the self.receivers list has dead weakrefs. If so, we will # clean those up in connect, disconnect and _live_receivers while # holding self.lock. Note that doing the cleanup here isn't a good - # idea, _remove_receiver() will be called as side effect of garbage + # idea, _flag_dead_receivers() will be called as side effect of garbage # collection, and so the call can happen while we are already holding # self.lock. self._dead_receivers = True diff --git a/tests/dispatch/tests.py b/tests/dispatch/tests.py index 18426d8dd1..e91d29abdd 100644 --- a/tests/dispatch/tests.py +++ b/tests/dispatch/tests.py @@ -1,7 +1,9 @@ import weakref from types import TracebackType +from unittest import mock from django.dispatch import Signal, receiver +from django.dispatch.dispatcher import _make_id from django.test import SimpleTestCase from django.test.utils import garbage_collect, override_settings @@ -75,7 +77,15 @@ class DispatcherTests(SimpleTestCase): a_signal.disconnect(receiver_1_arg, sender=object) self.assertTestIsClean(a_signal) - def test_garbage_collected(self): + def test_unweakrefable_sender(self): + sender = object() + a_signal.connect(receiver_1_arg, sender=sender) + result = a_signal.send(sender=sender, val="test") + self.assertEqual(result, [(receiver_1_arg, "test")]) + a_signal.disconnect(receiver_1_arg, sender=sender) + self.assertTestIsClean(a_signal) + + def test_garbage_collected_receiver(self): a = Callable() a_signal.connect(a.a, sender=self) del a @@ -84,6 +94,41 @@ class DispatcherTests(SimpleTestCase): self.assertEqual(result, []) self.assertTestIsClean(a_signal) + def test_garbage_collected_sender(self): + signal = Signal() + + class Sender: + pass + + def make_id(target): + """ + Simulate id() reuse for distinct senders with non-overlapping + lifetimes that would require memory contention to reproduce. + """ + if isinstance(target, Sender): + return 0 + return _make_id(target) + + def first_receiver(attempt, **kwargs): + return attempt + + def second_receiver(attempt, **kwargs): + return attempt + + with mock.patch("django.dispatch.dispatcher._make_id", make_id): + sender = Sender() + signal.connect(first_receiver, sender) + result = signal.send(sender, attempt="first") + self.assertEqual(result, [(first_receiver, "first")]) + + del sender + garbage_collect() + + sender = Sender() + signal.connect(second_receiver, sender) + result = signal.send(sender, attempt="second") + self.assertEqual(result, [(second_receiver, "second")]) + def test_cached_garbaged_collected(self): """ Make sure signal caching sender receivers don't prevent garbage