Opened 10 years ago

Closed 8 years ago

#599 closed defect (fixed)

locmem cache should deepcopy values from the cache to prevent aliasing

Reported by: hugo Owned by: jacob
Component: Core (Cache system) Version:
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The locmem cache currently returns the objects from the cache directly. But it is based on a in-memory dictionary and so the returned response could be changed outside and the change would be made to the cached object itself. Think for example about a middleware that changes the response headers - if that is fed a response from the locmem:/// cache, it will change the cached response itself and the next cache hit will deliver a changed object.

This patch should solve that problem:

Index: django/core/cache.py
===================================================================
--- django/core/cache.py        (revision 804)
+++ django/core/cache.py        (working copy)
@@ -230,6 +230,7 @@
     import cPickle as pickle
 except ImportError:
     import pickle
+import copy
 from django.utils.synch import RWLock
 
 class _LocMemCache(_SimpleCache):
@@ -250,7 +251,7 @@
             elif exp < now:
                 should_delete = True
             else:
-                return self._cache[key]
+                return copy.deepcopy(self._cache[key])
         finally:
             self._lock.reader_leaves()
         if should_delete:

The CacheMiddleware itself currently does a copy.copy() on the cached response, but that only is a shallow copy - and shallow copies don't help with the above header-changing scenario. I think with this patch, the copy.copy() in the CacheMiddleware can be dropped, as the other caches all use pickling and unpickling to store and retrieve the objects (and the memcache interface does it's own pickling/unpickling) and so already do something similar to deep copying.

Change History (5)

comment:1 Changed 10 years ago by eugene@…

+1. That's why my original implementation used pickling for locmem and other caches. Otherwise I was getting responses with inconsistent headers, e.g., compressed content without proper header.

comment:2 Changed 10 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

(In [821]) Fixed #599 -- locmem cache now uses deepcopy() to prevent aliasing. Thanks, Hugo

comment:3 Changed 8 years ago by nick@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

Under typical caching scenarios (e.g. write a few times read many times), would it be more efficient to do the (expensive) deep copy when setting the cache entry rather than every time it is retrieved?

comment:4 Changed 8 years ago by nick@…

Sorry, I half take that last comment back. If the object is to avoid unintended alteration of the cached value, then deepcopy is required on 'get'; however, if the app developer knows not to change the retrieved value (or to copy it explicitly if changing it), then in scenarios where large objects are stored, deepcopy in 'get' will be potentially expensive.

comment:5 Changed 8 years ago by Simon G. <dev@…>

  • Resolution set to fixed
  • Status changed from reopened to closed

I'm closing this ticket - the original reason it was opened has been fixed. Nick, if you think you can improve things here, please open a new ticket (and a patch would be nice! ;)

Note: See TracTickets for help on using tickets.
Back to Top