Re: Is using version 2 of the pickle protocol in {DB,FileBased}Cache

[Paul McMillan]

Thanks for filing the bug.

It's probably best to discuss higher-level stuff on the mailing list,
and details of the ticket on the ticket. That said, there's a lot of
overlap.

While setting pickle to use a lower protocol would "fix" the problem,
it's really only a bandaid. Lower versions of the pickle protocol are
badly inefficient with new-style classes, and should be avoided at all
costs. We really just need to stop trying to pickle a class that is
unpicklable.

The appropriate fix for this probably lies in serializing the
SimpleCookie object back into a string (that's what cookies are
anyway, right?) before we pass it to pickle. I haven't looked closely
at the code there, but that's where I would start.

As for the test, you probably want to test it more holistically -
maybe a test that fetches a page in the test client several times in a
row and makes sure that it is the same each time.

Further discussion should probably happen on the ticket.

-Paul

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Is using version 2 of the pickle protocol in {DB,FileBased}Cache

[Raphael Kubo da Costa]

Paul McMillan  writes:

> Ah, this does sound like a pretty nasty issue with our code then.
> Unfortunately, the test suite doesn't get run as often as it should
> with the various cache backends enabled, and I imagine that may be how
> this cropped up. There have been a number of similar bugs in the past
> - caching is hard! I'd appreciate it if you could open a ticket for
> this and reference the mailing list discussion. If you could set the
> owner to me (PaulM), I'll work on it when I get a chance. Thanks for
> taking the time to track down how this is happening.

Hey there, Paul,

Thanks for the interest, I've filed bug #15863.

In case you have some ideas on the directions to take, I could try
working on a patch to tackle the issue -- from what I see, using
pickle.HIGHEST_PROTOCOL is causing the bug, however using a value <
pickle.HIGHEST_PROTOCOL is also unwanted due to the serialized string
being much larger?

In case I work on a patch to improve the unit tests for this part,
should I file a new ticket? Should this mail have actually been sent as
a comment to the ticket? :)

-- 
Raphael Kubo da Costa
ProFUSION embedded systems
http://profusion.mobi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Is using version 2 of the pickle protocol in {DB,FileBased}Cache

[Paul McMillan]

Ah, this does sound like a pretty nasty issue with our code then.
Unfortunately, the test suite doesn't get run as often as it should
with the various cache backends enabled, and I imagine that may be how
this cropped up. There have been a number of similar bugs in the past
- caching is hard! I'd appreciate it if you could open a ticket for
this and reference the mailing list discussion. If you could set the
owner to me (PaulM), I'll work on it when I get a chance. Thanks for
taking the time to track down how this is happening.

-Paul

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Is using version 2 of the pickle protocol in {DB,FileBased}Cache

[Raphael Kubo da Costa]

Paul McMillan  writes:

> Yes, SimpleCookie is known to be an unpickleable class. We shouldn't
> be directly pickling it anywhere in Django. In your code, you should
> probably turn the cookie into a string before caching it. I'm not
> clear if the bug you're experiencing is happening in Django's code or
> something your application is doing directly with SimpleCookie.

[snip]

> I think that your provided test case is trying to do something that is
> explicitly not supported, but I'm unclear on whether or not there is
> an issue in Django-provided code. Could you provide a little more
> information?

Hi Paul,

I am not trying to pickle SimpleCookie directly -- in fact, I enabled
the cache middlewares in settings.py and then set CACHE_BACKEND to
'file:///some/directory'.

I then had a view with no specific cache decorators, but since the
session backend is also on it added the `Vary: Cookie' header.

After that, I started noticing that a login page including the
`csrf_token' tag started repeating the token when I used curl to access
it without providing any cookies or login credentials. And after the
first time I accessed it, the Set-Cookie header started misbehaving like
it did in the test case I attached -- instead of looking like

  Set-Cookie: foo=bar; other-parameters;

it was looking like

  Set-Cookie: foo="Set-Cookie: foo=bar; other-parameters;"

and the value in the csrf tag was being expanded to something along the
lines of "Set-Cookie: foo=bar; other-parameters;", so validation failed
later.

Some investigation led me to find the problem in the cache backend I was
using, as the cookies inside the cached HttpResponse were being
serialized incorrectly and later picked by FetchFromCacheMiddleware.

-- 
Raphael Kubo da Costa
ProFUSION embedded systems
http://profusion.mobi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Is using version 2 of the pickle protocol in {DB,FileBased}Cache

[Paul McMillan]

Hi Raphael,

Yes, SimpleCookie is known to be an unpickleable class. We shouldn't
be directly pickling it anywhere in Django. In your code, you should
probably turn the cookie into a string before caching it. I'm not
clear if the bug you're experiencing is happening in Django's code or
something your application is doing directly with SimpleCookie.

The last memcached backend I looked at pickled things using the
highest protocol. I don't remember which one it was. It's very odd
that you're not seeing the same error using memcached.

Locmem should use the highest version of pickle, to more closely
mirror the behavior of memcached. I'll open a ticket about that.
Django shouldn't switch to the earlier versions of pickle for
performance reasons.

I think that your provided test case is trying to do something that is
explicitly not supported, but I'm unclear on whether or not there is
an issue in Django-provided code. Could you provide a little more
information?

Thanks,
-Paul

On Fri, Apr 15, 2011 at 1:23 PM, Raphael Kubo da Costa
 wrote:
> Hello there,
>
> I was experiencing some problems with Django's caching system on 1.2.X
> (1.2.5, to be more specific) when using either the database or the
> file-based backends.
>
> Both use pickle and call pickle.dumps(..., pickle.HIGHEST_PROTOCOL) when
> serializing the data after being called by UpdateCacheMiddleware.
>
> However, it looks like pickle does not play nice with SimpleCookie-based
> classes [1]. In the first request (still not cached), the csrf token is
> set correctly as follows:
>
>  Set-Cookie:  csrftoken=XX; Max-Age=31449600; Path=/
>
> When the same view is requested again, though, the cookie is retrieved
> incorrectly from the cache by FetchFromCacheMiddleware:
>
>  Set-Cookie:  csrftoken="Set-Cookie: csrftoken=XX Max-Age=31449600\073 Path=/"
>
> The locmem, dummy and memcached backends do not present this problem:
> locmem does not specify the protocol version when calling pickle.dumps,
> which means protocol version 0 will be used; dummy does not do anything
> and memcached does not use pickle. Pickle protocol versions 0 and 1 work
> fine.
>
> The following patch to the unit tests should break both
> FileBasedCacheTests and DBCacheTests. I only tested it against the 1.2.X
> git branch, but 1.3.X should present the same behaviour, and both Python
> 2.7.1 and Python 3.2 fail.
>
> diff --git a/tests/regressiontests/cache/tests.py 
> b/tests/regressiontests/cache/tests.py
> index 0581e4e..5611eef 100644
> --- a/tests/regressiontests/cache/tests.py
> +++ b/tests/regressiontests/cache/tests.py
> @@ -285,6 +285,22 @@ class BaseCacheTests(object):
>         self.assertEqual(self.cache.get("expire2"), "newvalue")
>         self.assertEqual(self.cache.has_key("expire3"), False)
>
> +    def test_cookie_caching(self):
> +        try:
> +            from Cookie import SimpleCookie
> +        except ImportError:
> +            from http.cookies import SimpleCookie
> +
> +        test_cookie = SimpleCookie()
> +        test_cookie['key'] = 'some value'
> +
> +        self.cache.set('some_cookie', test_cookie)
> +
> +        cached_cookie = self.cache.get('some_cookie')
> +
> +        self.assertEqual(cached_cookie['key'].value,
> +                         test_cookie['key'].value)
> +
>     def test_unicode(self):
>         # Unicode values can be cached
>         stuff = {
>
> [1] http://bugs.python.org/issue826897
>
> --
> Raphael Kubo da Costa
> ProFUSION embedded systems
> http://profusion.mobi
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Django developers" group.
> To post to this group, send email to django-developers@googlegroups.com.
> To unsubscribe from this group, send email to 
> django-developers+unsubscr...@googlegroups.com.
> For more options, visit this group at 
> http://groups.google.com/group/django-developers?hl=en.
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Is using version 2 of the pickle protocol in {DB,FileBased}Cache

[Raphael Kubo da Costa]

Hello there,

I was experiencing some problems with Django's caching system on 1.2.X
(1.2.5, to be more specific) when using either the database or the
file-based backends.

Both use pickle and call pickle.dumps(..., pickle.HIGHEST_PROTOCOL) when
serializing the data after being called by UpdateCacheMiddleware.

However, it looks like pickle does not play nice with SimpleCookie-based
classes [1]. In the first request (still not cached), the csrf token is
set correctly as follows:

  Set-Cookie:  csrftoken=XX; Max-Age=31449600; Path=/

When the same view is requested again, though, the cookie is retrieved
incorrectly from the cache by FetchFromCacheMiddleware:

  Set-Cookie:  csrftoken="Set-Cookie: csrftoken=XX Max-Age=31449600\073 Path=/"

The locmem, dummy and memcached backends do not present this problem:
locmem does not specify the protocol version when calling pickle.dumps,
which means protocol version 0 will be used; dummy does not do anything
and memcached does not use pickle. Pickle protocol versions 0 and 1 work
fine.

The following patch to the unit tests should break both
FileBasedCacheTests and DBCacheTests. I only tested it against the 1.2.X
git branch, but 1.3.X should present the same behaviour, and both Python
2.7.1 and Python 3.2 fail.

diff --git a/tests/regressiontests/cache/tests.py 
b/tests/regressiontests/cache/tests.py
index 0581e4e..5611eef 100644
--- a/tests/regressiontests/cache/tests.py
+++ b/tests/regressiontests/cache/tests.py
@@ -285,6 +285,22 @@ class BaseCacheTests(object):
 self.assertEqual(self.cache.get("expire2"), "newvalue")
 self.assertEqual(self.cache.has_key("expire3"), False)
 
+def test_cookie_caching(self):
+try:
+from Cookie import SimpleCookie
+except ImportError:
+from http.cookies import SimpleCookie
+
+test_cookie = SimpleCookie()
+test_cookie['key'] = 'some value'
+
+self.cache.set('some_cookie', test_cookie)
+
+cached_cookie = self.cache.get('some_cookie')
+
+self.assertEqual(cached_cookie['key'].value,
+ test_cookie['key'].value)
+
 def test_unicode(self):
 # Unicode values can be cached
 stuff = {

[1] http://bugs.python.org/issue826897

-- 
Raphael Kubo da Costa
ProFUSION embedded systems
http://profusion.mobi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.