Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#7233 closed (fixed)

placing request.POST.copy() in session wipes all session values

Reported by: mikechambers Owned by: nobody
Component: contrib.sessions Version: master
Severity: Keywords:
Cc: rajesh.dhawan@…, dsalvetti@…, philipp@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description (last modified by ramiro)

Version : (0, 97, 'pre')

The issue is that it appears trying to store a QueryDict (request.POST, or a copy of a QueryDict) in the session, will wipe all session data.

You can see the original thread here:

http://groups.google.com/group/django-users/browse_thread/thread/260d9b94f773655e

Basically:

request.session['form_post_data'] = request.POST.copy()
request.session['foo'] = "bar"

Then, in another request:

print request.session.keys()

prints []

(i.e. no keys)

But:

request.session['foo'] = "bar"

then in another request:

print request.session.keys()
prints ['foo']

There seems to be two potential issues:

  1. Cannot store request.POST.copy() or request.POST in the session (should you be able to do this?)
  2. Trying to place request.POST.copy() in the session wipes all session values.

More info on the following threads:

http://groups.google.com/group/django-users/browse_thread/thread/260d9b94f773655e

and

http://groups.google.com/group/django-developers/browse_thread/thread/ec651a3b5f877698

Attachments (3)

sessiontest.zip (11.4 KB) - added by mikechambers 7 years ago.
test project that reproduces issue
querydict_pickle_fix.diff (1.9 KB) - added by rajesh.dhawan@… 7 years ago.
Fix to make copies of QueryDict instances pickle/unpickle friendly
picklable_QueryDict.diff (518 bytes) - added by rajeshdhawan 7 years ago.
Simpler patch to make QueryDict unpickle friendly

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by mikechambers

test project that reproduces issue

Changed 7 years ago by rajesh.dhawan@…

Fix to make copies of QueryDict instances pickle/unpickle friendly

comment:1 follow-up: Changed 7 years ago by rajeshdhawan

  • Cc rajesh.dhawan@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The problem is that django.http.QueryDict instances (even mutable copies of them) do not unpickle properly because the unpickle mechanism does not called init which makes such instances devoid of attributes _mutable and encoding. Without these two attributes pretty much all methods of the resulting instance are unusable. In particular _assert_mutable() crashes and prevents setitem() from working.

The bottom line is that these instances do get pickled and stored into the session but when the session is restored and the object is being unpickled that fails. The session mechanism simply disregards this (rightfully) and returns an empty session dictionary. So no other session data is readable.

I've attached a patch that *hopefully* fixes this. The patch ensures that _mutable and encoding values are saved in the pickled stream and are restored back during unpickling.

comment:2 follow-up: Changed 7 years ago by Alex

If I understand it correctly couldn't you just implement __getstate__ and __setstate__ methods on QueryDict.

comment:3 in reply to: ↑ 2 Changed 7 years ago by rajeshdhawan

Replying to Alex:

If I understand it correctly couldn't you just implement __getstate__ and __setstate__ methods on QueryDict.

Normally, that would work. My original attempt to fix this was indeed to implement those methods only to discover that because QueryDict.__setitem__ bombs (on the above mentioned _mutable assert check), __setstate__ fails during unpickling.

comment:4 in reply to: ↑ 1 ; follow-up: Changed 7 years ago by mikechambers

Replying to rajeshdhawan:

I just tested the patch, and you can now store a copy of QueryDict in the session.

This now works:

	request.session['post_copy'] = request.POST.copy()

You cannot place the POST QueryDict directly into the session like so:

	request.session['post'] = request.POST

I am not sure if that is the intended behavior (although I am guessing it is)

comment:5 in reply to: ↑ 4 Changed 7 years ago by rajeshdhawan

Replying to mikechambers:

You cannot place the POST QueryDict directly into the session like so:

	request.session['post'] = request.POST

I am not sure if that is the intended behavior (although I am guessing it is)

I think that should be the intended behaviour since request.POST is documented to be immutable. The docs recommend using request.POST.copy() if you need a mutable clone that you can modify (and, I assume, pickle, save in session, restore from session, etc.)

comment:6 Changed 7 years ago by anonymous

  • Needs tests set

comment:7 Changed 7 years ago by Simon Greenhill

  • Triage Stage changed from Unreviewed to Accepted

comment:8 Changed 7 years ago by ramiro

  • Description modified (diff)

comment:9 Changed 7 years ago by djoume

  • Cc dsalvetti@… added

comment:10 Changed 7 years ago by philwo

  • Cc philipp@… added
  • milestone set to 1.0

I just tripped over this bug too.. debugged my code, then Djangos code for two hours, came to the same conclusions as the others here. This is really bad, to swallow all exceptions occuring on unpickling the session-data, even in DEBUG = True mode! It creates extremely hard to find errors, because you debug your code for ages and don't see anything wrong - and then you realize that Django clears your session on the next request.. you debug that too, think it's a bug in Django .. and THEN realize you were doing something not allowed, but Django never complained by an exception, but just kept quiet and cleared your session on the next request.

So, it would be really cool to:

  • Throw exceptions on unpickle-errors to the developer and don't silently discard them (at least not in Debug mode!)
  • Make QuerySets pickle-able (why is request.POST a QueryDict at all?)

comment:11 Changed 7 years ago by mtredinnick

  • Patch needs improvement set

I don't really like the approach in this patch because it's effectively creating a new class object with the appropriate mutable setting, rather than a class instance. In other words, it feels like __new__ is being used for the wrong purpose and __init__ (or similar) should really be used. A solution based on __setstate__ and __getstate__ would be preferred. Rajesh mentioned a problem then with __setitem__, but I don't see why __setstate__ can't set the _mutable attribute as almost the first thing it does to work around that. I'll try to have a poke at this, probably after the beta release, but if anybody wants to get there first, I'd be looking hard at the __getstate__/__setstate__ approach.

Also, this really, really needs a test case so that we can assure it doesn't regress. The data structure is already tested in tests/regressiontests/httpwrappers/, so extra things can go in there.

comment:12 Changed 7 years ago by rajeshdhawan

Malcolm, your instincts are dead on. The fix here should be much simpler than what my original patch does. So I had another look at this and am attaching a patch that seems to do the trick while being minimal. It simply defines _mutable (and encoding) with default values outside of __init__ so that the session unpickler doesn't choke on trying to find those attributes.

Changed 7 years ago by rajeshdhawan

Simpler patch to make QueryDict unpickle friendly

comment:13 Changed 7 years ago by copelco

rajeshdhawan's patch worked for me

comment:14 Changed 7 years ago by mtredinnick

This patch still doesn't completely solve the problem. There's an issue that the order in which attributes are pickled depends upon dictionary hashing order, which varies from platform to platform. On the machine I'm on right now, for example, I can happily pickle and unpickle QuerySets, mutable or immutable, without any problems.

It seems that in some cases, however, the _mutable attribute is pickled (and therefore restored) after some of the dictionary items. Take that one step further and you realise that it's also possible for encoding to be pickled after those values, which means that it will have a value of None during calls to __setitem__(), which will lead to an exception being raised by unicode(), since that cannot have a second parameter value of None.

I've got a patch that works around that, but I can't commit it right this minute because I'm having trouble reaching the subversion repo over the network I'm on. Will commit it tomorrow morning. Mostly making a note here so that another committer doesn't accidentally duplicate the thinking and work.

comment:15 Changed 7 years ago by mtredinnick

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

(In [8460]) Fixed #7233 -- Ensured that QueryDict classes are always unpicklable. This
problem only arose on some systems, since it depends upon the order in which
the attributes are pickled. Makes reliable testing kind of tricky.

comment:16 Changed 7 years ago by mtredinnick

Just in case anybody is reading this in the future, my analysis here was partially bogus. The real issue with caching and pickling is that we use pickle protocol 2 when caching, which changes the behaviour of how things are unpickled. My initial tests were only with the default (level 0?) pickle protocol. In any case, it's still fixed with the above commit.

comment:17 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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