Issue #1134 new

cherrypy.request.body.read() gives 500 for GET requests

Jim Paris
created an issue

This server: {{{

!python

import cherrypy class Root(object): @cherrypy.expose def upload(self): body = cherrypy.request.body.read() return "got %d bytes, thanks\n" % len(body) cherrypy.config.update({ 'server.socket_host': '127.0.0.1', 'server.socket_port': 1234, }) cherrypy.quickstart(Root()) }}} with a HTTP PUT works fine: {{{ $ curl -T <(echo hello) http://localhost:1234/upload got 6 bytes, thanks }}} but gives a 500 Internal Server Error with a GET: {{{ $ curl http://localhost:1234/upload <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"></meta> <title>500 Internal Server Error</title> <style type="text/css"> #powered_by { margin-top: 20px; border-top: 2px solid black; font-style: italic; }

#traceback {
    color: red;
}
</style>

</head> <body> <h2>500 Internal Server Error</h2> <p>The server encountered an unexpected condition which prevented it from fulfilling the request.</p> <pre id="traceback">Traceback (most recent call last): File "/usr/lib/python2.7/dist-packages/cherrypy/_cprequest.py", line 656, in respond response.body = self.handler() File "/usr/lib/python2.7/dist-packages/cherrypy/lib/encoding.py", line 188, in call self.body = self.oldhandler(*args, kwargs) File "/usr/lib/python2.7/dist-packages/cherrypy/_cpdispatch.py", line 34, in __call__ return self.callable(*self.args, self.kwargs) File "server.py", line 7, in upload body = cherrypy.request.body.read() File "/usr/lib/python2.7/dist-packages/cherrypy/_cpreqbody.py", line 455, in read return self.fp.read(size, fp_out) TypeError: read() takes at most 2 arguments (3 given) </pre> <div id="powered_by"> <span>Powered by <a href="http://www.cherrypy.org">CherryPy 3.2.2</a></span> </div> </body> </html> }}} The following patch seems to fix it: {{{ diff -r 2934ecf8075b cherrypy/_cpreqbody.py --- a/cherrypy/_cpreqbody.py Fri Mar 09 16:26:05 2012 -0500 +++ b/cherrypy/_cpreqbody.py Mon Mar 26 12:42:27 2012 -0400 @@ -452,7 +452,7 @@ doc="""A deprecated alias for :attr:content_type<cherrypy._cpreqbody.Entity.content_type>.""")

 def read(self, size=None, fp_out=None):
  • return self.fp.read(size, fp_out)
  • return self.fp.read(size)

    def readline(self, size=None): return self.fp.readline(size) }}} Example after patch: {{{ $ curl -T <(echo hello) http://localhost:1234/upload got 6 bytes, thanks $ curl http://localhost:1234/upload got 0 bytes, thanks }}}

Comments (6)

  1. Jason R. Coombs

    We probably shouldn't remove the use of fp_out without first understanding why it was there in the first place. I removed the fp_out parameter from the `.read()` signature itself and ran the test suite, but there were no failures. I can find no documentation on the purpose of that parameter. I wonder under what circumstances fp_out has meaning. Neither Python file objects nor StringIO objects accept this parameter.

    I guess the next step is to run blame on the code and trace back the code change to determine why it was added.

  2. Robert Brewer

    The fp_out argument is there to support low-level code that reads the body in chunks, allowing each chunk to be written to a file rather than all chunks buffered in memory.

  3. Jim Paris reporter

    self.body is first initialized in _cprequest.py:respond() as:

                        self.body = _cpreqbody.RequestBody(
                            self.rfile, self.headers, request_params=self.params)
    

    This sets self.body.fp = self.rfile, which for a GET request is something like cherrypy.wsgiserver.wsgiserver2.KnownLengthRFile -- this does not expect or handle a fp_out parameter.

    If (and only if) the method is in self.methods_with_bodies, respond() continues by calling self.body.process(). This function replaces self.body.fp with SizedReader, which _does_ handle fp_out.

  4. space one

    don't apply this patch! this can be used: body = '' if cherrypy.request.method in cherrypy.request.methods_with_bodies: body = cherrypy.request.body.read()

  5. Jim Paris reporter

    Do you say "don't apply this patch" because you think the patch is wrong, or just because you think it's unnecessary? I still argue that the code is wrong as written and that line can never execute succesfully -- at that point in time, there is no way that the self.fp.read() function even takes a fp_out parameter.

  6. Log in to comment