Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LRUCache.__copy__ is still unsound #3886

Closed
cmanallen opened this issue Dec 20, 2024 · 0 comments · Fixed by #3887
Closed

LRUCache.__copy__ is still unsound #3886

cmanallen opened this issue Dec 20, 2024 · 0 comments · Fixed by #3887

Comments

@cmanallen
Copy link
Member

cmanallen commented Dec 20, 2024

How do you use Sentry?

Sentry Saas (sentry.io)

Version

latest git revision

Steps to Reproduce

Mutable references are copied -- not the underlying value. This is visible here:

new._data = self._data.copy()
Where we copy the _data attribute and not deepcopy.

class MutableObject:
    def __init__(self):
        self.count = 0

cache1 = LRUCache(max_size=2)
cache1.set(1, MutableObject())

cache2 = cache1.__copy__()
node = cache2.get(1)
node.count += 1

assert cache1.get(1).count == 0  # fail
assert cache2.get(1).count == 1

We should not be manually implementing __copy__ at all. This is a general helper class and should not assume copy semantics. I lay out this case in this comment and this comment.

Expected Result

Mutable objects do not pollute copied caches.

Actual Result

Mutable objects do pollute copied caches.

antonpirker added a commit that referenced this issue Dec 23, 2024
- Removes manual overrides of copy behavior and leaves it up to the caller.
    - E.g. a future use case may require a non-deepcopy. If we override copy they would have to remove the dunder copy, update every implementation which relies copy, before finally creating their own copy implementation.
- Deepcopies the flag buffer.
    - Though we do not cache mutable references yet we may soon and so this foot gun should be removed from possibility.
- Removes "copy" test coverage from `test_lru_cache.py`. We're no longer assuming copy usage and leave it up to the caller.
    - The existing test in `tests/test_scope.py` covers the cache pollution case [originally mentioned here](#3852).
    - The mutable cache pollution case is not covered because we do not currently cache mutable objects.

In general a generic class should assume as few implementation details as possible.  If we leave the existing copy method someone may assume copy semantics and rely on it in a way that is inappropriate.

Closes: #3886

Co-authored-by: Anton Pirker <[email protected]>
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 a pull request may close this issue.

1 participant