Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#17187 closed Bug (needsinfo)

Incompatibility with Cherokee webserver's FastCGI handling

Reported by: sciyoshi Owned by: sam@…
Component: HTTP handling Version: 1.3
Severity: Normal Keywords: cherokee flup fastcgi fcgi content-length uwsgi
Cc: sam@…, leos@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This issue is causing any POST request with zero content-length to throw an error (in flup of all places) when set up as a FastCGI backend to Cherokee.

Traceback:
File "/home/sciyoshi/django/django/core/handlers/wsgi.py" in _get_request
  196.             self._request = datastructures.MergeDict(self.POST, self.GET)
File "/home/sciyoshi/django/django/core/handlers/wsgi.py" in _get_post
  210.             self._load_post_and_files()
File "/home/sciyoshi/django/django/http/__init__.py" in _load_post_and_files
  289.             self._post, self._files = QueryDict(self.raw_post_data, encoding=self._encoding), MultiValueDict()
File "/home/sciyoshi/django/django/http/__init__.py" in _get_raw_post_data
  251.                 self._raw_post_data = self.read()
File "/home/sciyoshi/django/django/http/__init__.py" in read
  301.         return self._stream.read(*args, **kwargs)
File "/home/sciyoshi/env2/local/lib/python2.7/site-packages/flup/server/fcgi_base.py" in read
  161.                     self._waitForData()
File "/home/sciyoshi/env2/local/lib/python2.7/site-packages/flup/server/fcgi_base.py" in _waitForData
  147.         self._conn.process_input()
File "/home/sciyoshi/env2/local/lib/python2.7/site-packages/flup/server/fcgi_base.py" in process_input
  711.             self._do_unknown_type(rec)
File "/home/sciyoshi/env2/local/lib/python2.7/site-packages/flup/server/fcgi_base.py" in _do_unknown_type
  824.         self.writeRecord(rec)

Exception Type: NameError at /account/
Exception Value: global name 'rec' is not defined

This issue happens with the combination of flup==1.0.2, cherokee==1.2.101, and django==1.3. I've already submitted a patch to Cherokee which fixes this on their end, but the edge case may still happen with other servers (an extra FCGI record being sent after the PARAMs). There is a simple fix for this in Django as well, so I'm submitting it here for consideration.

Attachments (1)

17187.patch (763 bytes) - added by sciyoshi 2 years ago.

Download all attachments as: .zip

Change History (11)

Changed 2 years ago by sciyoshi

comment:1 Changed 2 years ago by sciyoshi

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by Alex

All I see is a NameError, which is clearly a bug in flup's code.

comment:3 Changed 2 years ago by sciyoshi

That's true, but at that point in the code content_length will always be zero, so there is no need to continue trying to read from the underlying stream.

comment:4 Changed 2 years ago by sciyoshi

Also, I would consider it a regression, since this has been broken since r14394 with no change to either flup or cherokee.

comment:5 Changed 2 years ago by aminland

  • Easy pickings set

I second this.
The bug in flup clode was fixed in June, although it's not available as a "release" yet.
See his commit here

I pulled the fix for flup, and what happened as a result is that the request would process in django, but I would get a Cherokee 500 error (ot comming from the django app).

Also this issue doesn't seem to affect only fcgi?, and not scgi?.

While the bug may stem from an issue with cherokee, it works fine in django 1.2, and broke in the commit listed above.

comment:6 Changed 2 years ago by Leo

  • Cc leos@… added

A similar issue happens when running against uWSGI. Django attempts to do a read() even if the content-length is explicitly set to zero. The result is that the read() call blocks. The repro in my case is really simple. Get the latest uWSGI, spin up simple django app, send a POST with nothing in it.

There's some good explanation of this issue in Graham Dumpleton's blog: http://blog.dscpl.com.au/2009/10/details-on-wsgi-10-amendmentsclarificat.html

Django should properly distinguish between unknown content-length (when trying to read might make sense) and one explicitly set to zero, when trying to read is a mistake. As per the WSGI spec, it would be wrong for uWSGI to return anything in response to a read() call, even an EOF when it set the content-length to 0.

Last edited 2 years ago by Leo (previous) (diff)

comment:7 Changed 2 years ago by Leo

  • Keywords uwsgi added

comment:8 Changed 2 years ago by Leo

  • Easy pickings unset

comment:9 Changed 2 years ago by aaugustin

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

It looks like this part of the code changed heavily since Django 1.3, the patch doesn't apply.

Based on code inspection, I'd say this problem can't happen anymore -- from WSGIRequest.__init__:

        try:
            content_length = int(self.environ.get('CONTENT_LENGTH'))
        except (ValueError, TypeError):
            content_length = 0
        self._stream = LimitedStream(self.environ['wsgi.input'], content_length)

If it still occurs on trunk, could you reopen the ticket and provide a test case? (or at least simple steps to reproduce the problem, involving as few third-party tools as possible).

comment:10 Changed 2 years ago by Leo

Thanks for looking at this. It looks like LimitedStream should prevent this from happening again.

The test case could easily be done with a mock, basically if the CONTENT_LENGTH is explicitly set to 0 in the request, Django should not call read() on self.environ['wsgi.input']. If however, the CONTENT_LENGTH is not properly specified or an invalid integer, Django may still call read() as per the wsgi discussion I linked to before.

It looks like the code changes have made it so that Django will not call read() if a CONTENT_LENGTH is not provided (or is invalid). That's probably the right action to take but should get noted in the release notes since it Django now behaves differently.

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.