Opened 16 years ago

Closed 16 years ago

#6063 closed (fixed)

BaseHttp server crash since last SVN update

Reported by: Martín Conte Owned by: Malcolm Tredinnick
Component: django-admin.py runserver Version: dev
Severity: Keywords:
Cc: jarek.zgoda@…, eugene.lazutkin@…, nesh@…, chris@…, janusz.harkot@…, gajon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (4)

basehttp_safestring.patch (811 bytes ) - added by Martin Conte 16 years ago.
Safestring is also an string ;)
basehttp_safestring_instance.patch (551 bytes ) - added by Gasper Zejn <zejn@…> 16 years ago.
Another patch, oneliner
httpresponse_only_str.diff (765 bytes ) - added by Jarek Zgoda 16 years ago.
Naive patch on HttpResponse to convert content to str (just to visualize the idea)
SafeString_remove.diff (476 bytes ) - added by janusz.harkot@… 16 years ago.
patch that is working for me

Download all attachments as: .zip

Change History (31)

by Martin Conte, 16 years ago

Attachment: basehttp_safestring.patch added

Safestring is also an string ;)

by Gasper Zejn <zejn@…>, 16 years ago

Another patch, oneliner

comment:1 by Gasper Zejn <zejn@…>, 16 years ago

Has patch: set

comment:2 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

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

comment:3 by Reto Aebersold, 16 years ago

Needs tests: set
Resolution: fixed
Status: closedreopened
Triage Stage: UnreviewedDesign decision needed

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.

comment:4 by Malcolm Tredinnick, 16 years ago

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.

comment:5 by Malcolm Tredinnick, 16 years ago

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.

in reply to:  5 comment:6 by Reto Aebersold, 16 years ago

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.

comment:7 by Reto Aebersold, 16 years ago

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

comment:8 by Jacob, 16 years ago

Triage Stage: Design decision neededAccepted

comment:9 by Malcolm Tredinnick, 16 years ago

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.

comment:10 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick
Status: reopenednew

comment:11 by denis <kuzmichyov@…>, 16 years ago

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

comment:12 by Jarek Zgoda, 16 years ago

Cc: jarek.zgoda@… added

comment:13 by Jarek Zgoda, 16 years ago

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

comment:14 by Malcolm Tredinnick, 16 years ago

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.

comment:15 by Jarek Zgoda, 16 years ago

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

comment:16 by aleander@…, 16 years ago

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

in reply to:  14 comment:17 by Jarek Zgoda, 16 years ago

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).

by Jarek Zgoda, 16 years ago

Attachment: httpresponse_only_str.diff added

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

comment:18 by Eugene Lazutkin <eugene.lazutkin@…>, 16 years ago

Cc: eugene.lazutkin@… added

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

comment:19 by Nebojsa Djordjevic - nesh, 16 years ago

Cc: nesh@… added

comment:20 by Chris Moffitt <Chris@…>, 16 years ago

Cc: chris@… added

by janusz.harkot@…, 16 years ago

Attachment: SafeString_remove.diff added

patch that is working for me

comment:21 by janusz.harkot@…, 16 years ago

Cc: janusz.harkot@… added

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

comment:22 by Gasper Zejn <zejn@…>, 16 years ago

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.).

comment:23 by janusz.harkot@…, 16 years ago

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...

comment:24 by Malcolm Tredinnick, 16 years ago

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.

comment:25 by Eugene Lazutkin <eugene.lazutkin@…>, 16 years ago

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.

comment:26 by Jorge Gajon, 16 years ago

Cc: gajon@… added

comment:27 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

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

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