Code

Opened 6 years ago

Closed 6 years ago

#6063 closed (fixed)

BaseHttp server crash since last SVN update

Reported by: Martín Conte Owned by: mtredinnick
Component: django-admin.py runserver Version: master
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: UI/UX:

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 6 years ago.
Safestring is also an string ;)
basehttp_safestring_instance.patch (551 bytes) - added by Gasper Zejn <zejn@…> 6 years ago.
Another patch, oneliner
httpresponse_only_str.diff (765 bytes) - added by zgoda 6 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@… 6 years ago.
patch that is working for me

Download all attachments as: .zip

Change History (31)

Changed 6 years ago by Martin Conte

Safestring is also an string ;)

Changed 6 years ago by Gasper Zejn <zejn@…>

Another patch, oneliner

comment:1 Changed 6 years ago by Gasper Zejn <zejn@…>

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by mtredinnick

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

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

comment:3 Changed 6 years ago by aeby

  • Needs tests set
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Design 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 Changed 6 years ago by mtredinnick

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 follow-up: Changed 6 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 6 years ago by aeby

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 Changed 6 years ago by aeby

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 Changed 6 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:9 Changed 6 years ago by mtredinnick

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 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Status changed from reopened to new

comment:11 Changed 6 years ago by 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.

comment:12 Changed 6 years ago by zgoda

  • Cc jarek.zgoda@… added

comment:13 Changed 6 years ago by zgoda

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

comment:14 follow-up: Changed 6 years ago by mtredinnick

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 Changed 6 years ago by zgoda

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

comment:16 Changed 6 years ago by 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].

comment:17 in reply to: ↑ 14 Changed 6 years ago by 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).

Changed 6 years ago by zgoda

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

comment:18 Changed 6 years ago by Eugene Lazutkin <eugene.lazutkin@…>

  • Cc eugene.lazutkin@… added

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

comment:19 Changed 6 years ago by nesh

  • Cc nesh@… added

comment:20 Changed 6 years ago by Chris Moffitt <Chris@…>

  • Cc chris@… added

Changed 6 years ago by janusz.harkot@…

patch that is working for me

comment:21 Changed 6 years ago by janusz.harkot@…

  • 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 Changed 6 years ago by 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.).

comment:23 Changed 6 years ago by 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...

comment:24 Changed 6 years ago by mtredinnick

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 Changed 6 years ago by 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.

comment:26 Changed 6 years ago by gajon

  • Cc gajon@… added

comment:27 Changed 6 years ago by mtredinnick

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.