Pull requests

#40 Merged
Repository
dandrzejewski dandrzejewski
Branch
default
Repository
cherrypy cherrypy
Branch
default

Fix for issue #1068 - unable to post more than 16KB over SSL

Author
  1. David Andrzejewski
Reviewers
Description

This hopefully resolves issue #1068. All I did was ensure that SSL_fileobject.recv() so that it never returns more than the number of bytes it was asked for, which is what the assert statement on line 1055 of wsgiserver2.py is checking on - that's where the failure occurs.

I'm not really sure if this is the best or most efficient way to do it, but this definitely appears to resolve it on my end, and it does not cause additional unit test failures.

I did not add unit tests for this because no unit tests are run in HTTPS mode, however I think someone with the required expertise should consider having the unit tests run in that mode.

Comments (5)

  1. Joel Rivera

    I have come to the realization after spending a shameful amount of time looking into this issue that the method SSL_fileobject.recv needs to be just to be just one line:

        def recv(self, size, *args, **kwargs):
            return self._safe_call(True, super(SSL_fileobject, self).recv, size, *args, **kwargs)
    

    Why are we buffering a call with a fixed size as an argument? I have make a couple of test with some 500MB files and is working fine, the call to self._sock.pending() is not required for the purpose of that method.

    BUT after thinking a little more I see that probably is a good idea to buffer the method and ask for pending plus verifying the size as you suggest. Probably the examples that I'm making are not a real use case because of the low latency of the localhost. Anyway the real issue is indeed in this method not in the caller in wsgiserver2.py, the thing that's bothers me is that on python3 is working fine and the almost exactly the same implementation (except for some extra locking ) in CP3.1 is also working fine.

    I'm going to look for a way to test the SSL part.

    This is the documentation of OpenSSLConection.

  2. David Andrzejewski author

    That is interesting that it works on Python 3.

    Anyway, I'm glad you think this is the right way to go.

    Per the OpenSSLConnection documentation (emphasis mine):

    recv(bufsize) Receive data from the Connection. The return value is a string representing the data received. The maximum amount of data to be received at once, is specified by bufsize.

    And that's what appears to be the problem - at least to my inexperienced eyes - more than the maximum amount of data might be retrieved for some reason. As you've seen, this will check for that and stop appending to the buffer if it's going to exceed the maximum.

  3. Joel Rivera

    Finally everything has fall into place, I do have a reason why it is working on CP3.1 and Python3 (this is pretty simple).

    1. This use to work on CP3.1 because the body of the request was read with readline which does not verify the length of the data that recv returns, also it always call recv with a fixed argument, the default read size of socket self._rbufsize which in my platform is 8192 in CP3.1 recv does return more than gets asked, almost always 2 * 8192 bytes, because of a coincidence of the record size of openssl -> http://repo.or.cz/w/mirror-openssl.git/blob/HEAD:/ssl/ssl2.h#l141

    2. This works in Python3 because the wsgiserver3.py does not use pyopenssl, and therefore does not usessl_pyopenssl.py :|, simple as that.

    The reason to not include any call to self._sock.pending to the fix is because that method only verifies that the underlying ssl connection does not have any buffered bytes of previously fetched records (e.g. records of 16383 bytes), and most important the buffering between records is already handled by SSL_read which is wrapped by pyOpenSSL at the SSLConnection.recv. In particular that recv method does not support the -1 and the existence of it is just to be thread safe, not to add more buffering magic.

    So the fix need to be just like this:

        def recv(self, size):
            return self._safe_call(True, super(SSL_fileobject, self).recv, size)
    

    David Andrzejewski It would be great If you could make the change to looks like that.

    And just for anyone interested in how is the call stack comparing CP3.1 to CP3.2:

    CP3.1:

      /usr/lib64/python2.7/threading.py(524)__bootstrap()
    -> self.__bootstrap_inner()
      /usr/lib64/python2.7/threading.py(551)__bootstrap_inner()
    -> self.run()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(1327)run()
    -> conn.communicate()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(1212)communicate()
    -> req.respond()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(572)respond()
    -> self._respond()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(584)_respond()
    -> response = self.wsgi_app(self.environ, self.start_response)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cptree.py(239)__call__()
    -> return app(environ, start_response)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cptree.py(130)__call__()
    -> return self.wsgiapp(environ, start_response)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpwsgi.py(313)__call__()
    -> return head(environ, start_response)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpwsgi.py(301)tail()
    -> return self.response_class(environ, start_response, self.cpapp)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpwsgi.py(74)__init__()
    -> self.setapp()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpwsgi.py(79)setapp()
    -> s, h, b = self.get_response()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpwsgi.py(219)get_response()
    -> response = self.request.run(meth, path, qs, rproto, headers, rfile)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cprequest.py(546)run()
    -> self.respond(pi)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cprequest.py(605)respond()
    -> self.process_body()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cprequest.py(727)process_body()
    -> keep_blank_values=1)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpcgifs.py(8)__init__()
    -> cgi.FieldStorage.__init__(self, *args, **kwds)
      /usr/lib64/python2.7/cgi.py(508)__init__()
    -> self.read_multi(environ, keep_blank_values, strict_parsing)
      /usr/lib64/python2.7/cgi.py(637)read_multi()
    -> environ, keep_blank_values, strict_parsing)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpcgifs.py(8)__init__()
    -> cgi.FieldStorage.__init__(self, *args, **kwds)
      /usr/lib64/python2.7/cgi.py(510)__init__()
    -> self.read_single()
      /usr/lib64/python2.7/cgi.py(647)read_single()
    -> self.read_lines()
      /usr/lib64/python2.7/cgi.py(669)read_lines()
    -> self.read_lines_to_outerboundary()
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/_cpcgifs.py(31)read_lines_to_outerboundary()
    -> line = self.fp.readline(1<<16)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(206)readline()
    -> data = self.rfile.readline(size)
      /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(900)readline()
    -> data = self.recv(self._rbufsize)
    > /tmp/cherrypy/env/lib/python2.7/site-packages/cherrypy/wsgiserver/__init__.py(1130)recv()
    -> buf = []
    

    CP3.2:

      /usr/lib64/python2.7/threading.py(524)__bootstrap()
    -> self.__bootstrap_inner()
      /usr/lib64/python2.7/threading.py(551)__bootstrap_inner()
    -> self.run()
      /mnt/files/repos/cherrypy/cherrypy/wsgiserver/wsgiserver2.py(1455)run()
    -> conn.communicate()
      /mnt/files/repos/cherrypy/cherrypy/wsgiserver/wsgiserver2.py(1312)communicate()
    -> req.respond()
      /mnt/files/repos/cherrypy/cherrypy/wsgiserver/wsgiserver2.py(834)respond()
    -> self.server.gateway(self).respond()
      /mnt/files/repos/cherrypy/cherrypy/wsgiserver/wsgiserver2.py(2153)respond()
    -> response = self.req.server.wsgi_app(self.env, self.start_response)
      /mnt/files/repos/cherrypy/cherrypy/_cptree.py(290)__call__()
    -> return app(environ, start_response)
      /mnt/files/repos/cherrypy/cherrypy/_cptree.py(147)__call__()
    -> return self.wsgiapp(environ, start_response)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(391)__call__()
    -> return head(environ, start_response)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(136)__call__()
    -> return _TrappedResponse(self.nextapp, environ, start_response, self.throws)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(149)__init__()
    -> self.response = self.trap(self.nextapp, self.environ, self.start_response)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(169)trap()
    -> return func(*args, **kwargs)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(96)__call__()
    -> return self.nextapp(environ, start_response)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(379)tail()
    -> return self.response_class(environ, start_response, self.cpapp)
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(222)__init__()
    -> self.run()
      /mnt/files/repos/cherrypy/cherrypy/_cpwsgi.py(320)run()
    -> request.run(meth, path, qs, rproto, headers, rfile)
      /mnt/files/repos/cherrypy/cherrypy/_cprequest.py(575)run()
    -> self.respond(pi)
      /mnt/files/repos/cherrypy/cherrypy/_cprequest.py(649)respond()
    -> self.body.process()
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(949)process()
    -> super(RequestBody, self).process()
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(515)process()
    -> proc(self)
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(214)process_multipart_form_data()
    -> process_multipart(entity)
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(208)process_multipart()
    -> part.process()
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(513)process()
    -> self.default_proc()
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(677)default_proc()
    -> self.file = self.read_into_file()
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(689)read_into_file()
    -> self.read_lines_to_boundary(fp_out=fp_out)
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(620)read_lines_to_boundary()
    -> line = self.fp.readline(1<<16)
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(824)readline()
    -> data = self.read(chunksize)
      /mnt/files/repos/cherrypy/cherrypy/_cpreqbody.py(788)read()
    -> data = self.fp.read(chunksize)
      /mnt/files/repos/cherrypy/cherrypy/wsgiserver/wsgiserver2.py(332)read()
    -> data = self.rfile.read(size)
      /mnt/files/repos/cherrypy/cherrypy/wsgiserver/wsgiserver2.py(1043)read()
    -> data = self.recv(left)
    > /mnt/files/repos/cherrypy/cherrypy/wsgiserver/ssl_pyopenssl.py(101)recv()
    -> buf = []