BaseHttp server crash since last SVN update

[Martín Conte]

I'm getting

Traceback (most recent call last):

  File "/usr/lib/python2.5/site-packages/django/core/servers/basehttp.py", line 279, in run
    self.finish_response()

  File "/usr/lib/python2.5/site-packages/django/core/servers/basehttp.py", line 318, in finish_response
    self.write(data)

  File "/usr/lib/python2.5/site-packages/django/core/servers/basehttp.py", line 389, in write
    assert type(data) is StringType,"write() argument must be string"

AssertionError: write() argument must be string

It is happens sice last SVN update. I think this is related to new SafeString patch. There is my patch.

[Martin Conte]

Safestring is also an string ;)

[Gasper Zejn <zejn@…>]

Another patch, oneliner

[Malcolm Tredinnick]

(In [6780]) Fixed some type checks in the development server. Fixed #6063.

[Reto Aebersold]

This fix only works for the BaseHTTPServer. It is not possible to use FastCGI with ​flup because there is also an assert inside the BaseFCGIServer write() method:

assert type(data) is str, 'write() argument must be string'

The fix should ensure that only str types are sent to the write() method.

[Malcolm Tredinnick]

No, the fix really shouldn't do that. SafeString is a subclass of str and therefore perfectly safe to pass in. I'll make the same change to the fcgi server as I did to base server.

[Malcolm Tredinnick]

Oh, I see what you're saying: the assert is in flup itself. That's just broken. They should be using isinstance() so that valid subclassing is supported. :-(

This is going to unbelievably hard to fix in Django and we're not actually doing anything wrong. Needs some thought, but filing a bug report with flup would be a help.

[Reto Aebersold]

Replying to mtredinnick:

Oh, I see what you're saying: the assert is in flup itself. That's just broken. They should be using isinstance() so that valid subclassing is supported. :-(

This is going to unbelievably hard to fix in Django and we're not actually doing anything wrong. Needs some thought, but filing a bug report with flup would be a help.

Ok, I will send a change request to flup.

[Reto Aebersold]

Allan Saddi gave the input that the usage of a string subclass is maybe not ​spec-compliant:
​http://trac.saddi.com/flup/ticket/28

[Malcolm Tredinnick]

Can somebody please give me a short example of how to repeat this problem. I can't see how a SafeString instance is leaking out of HttpResponse.

[denis <kuzmichyov@…>]

To reproduce the problem it is enough to run trunk copy of django as fastcgi server and setup some flat page for example.

[Jarek Zgoda]

The change that causes the problem was introduced in [6778], [6777] works fine.

[Malcolm Tredinnick]

Did [6807] fix this by any chance? I still can't see from reading the code what is causing the problem and I'm not easily able to set up the whole fcgi stuff on my laptop at the moment. So if somebody could test that commit and see, I'd appreciate it.

For anybody wanting to help out here, the thing to work out is where the SafeData is coming from. HttpResponse.content should always have type "str", not SafeString, so where is the bad type leaking from? We aren't about to revert [6778], since it solves another problem, thus we need to plug the leak after we find it.

[Jarek Zgoda]

I can confirm this problem still exists in [6844].

[aleander@…]

In newforms-admin, I worked around the problem by replacing source:django/branches/newforms-admin/django/template/__init__.py with the version from [6777].

[Jarek Zgoda]

Replying to mtredinnick:

For anybody wanting to help out here, the thing to work out is where the SafeData is coming from. HttpResponse.content should always have type "str", not SafeString, so where is the bad type leaking from? We aren't about to revert [6778], since it solves another problem, thus we need to plug the leak after we find it.

Well, it looks like Template render() method returns SafeString which in turn is passed to HttpResponse constructor. The most sensible point of ensuring only strings are leaving Django realm seems to me the HttpResponse's constructor (the content should be rendered and escaped anyway, so there's no need to have SafeString here).

[Jarek Zgoda]

Naive patch on HttpResponse to convert content to str (just to visualize the idea)

[Eugene Lazutkin <eugene.lazutkin@…>]

Problem still exists. It is easy to reproduce with flup (manage.py runfcgi) --- any 404 triggers the error.

[janusz.harkot@…]

patch that is working for me

[janusz.harkot@…]

Hi, I've just posted a very simple patch that is working for me, please check if you get this error message somewhere else.

[Gasper Zejn <zejn@…>]

To be honest, I don't really see a problem with isinstance.

The isinstance doc says, that if given a type as a second option, it checks whether type matches:

>>> print isinstance.__doc__
isinstance(object, class-or-type-or-tuple) -> bool

Return whether an object is an instance of a class or of a subclass thereof.
With a type as second argument, return whether that is the object's type.
The form using a tuple, isinstance(x, (A, B, ...)), is a shortcut for
isinstance(x, A) or isinstance(x, B) or ... (etc.).

[janusz.harkot@…]

I belive that we do not have any problem with it, but asaddi from ​http://trac.saddi.com/flup/ticket/28 has :)
so there always is a choice... convince him, include modified version of flup withing django tree (painful) or switch to different implementation :)
IMHO easiest is to patch django...

[Malcolm Tredinnick]

The problem with isinstance() is that it doesn't test purely is something is the type, but also (correctly and in a natural Python fashion) allows subclasses. I will argue that the WSGI spec is completely broken by insisting on a str type in that place, rather than also permitting something that is a subclass of string. Which part of duck-typing don't they understand? However, that is beside the point and we should comply with the spec whenever possible.

[Eugene Lazutkin <eugene.lazutkin@…>]

Hear, hear! We can sit and complain about the standard, or we can easily fix it on our side. I agree with Malcolm totally --- let's fix it to comply to the spec. If the spec is updated, we'll update our code as well.

[Malcolm Tredinnick]

(In [6895]) Fixed #6063 -- Caught one place in HttpResponse that was not returning a str
type (violation of WSGI spec). Thanks, janusz.harkot@….