New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Another int overflow on waitress Buffers #47

Closed
marcinkuzminski opened this Issue Nov 15, 2013 · 9 comments

Comments

Projects
None yet
2 participants
@marcinkuzminski
Member

marcinkuzminski commented Nov 15, 2013

So it's another case of #22, now on waitress 0.8.7

Org exception was:

Traceback (most recent call last): 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py", line 332, in service 
    task.service() 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/task.py", line 173, in service 
    self.execute() 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/task.py", line 420, in execute 
    self.write(chunk) 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/task.py", line 303, in write 
    channel.write_soon(towrite) 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py", line 314, in write_soon 
    self.outbufs[-1].append(data) 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/buffers.py", line 244, in append 
    sz = len(buf) 
OverflowError: long int too large to convert to int 
2013-11-14 17:24:28.508 ERROR [waitress] Unexpected exception when flushing 
Traceback (most recent call last): 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py", line 137, in handle_write 
    flush() 
  File "/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py", line 236, in _flush_some 
    outbuflen = len(outbuf) 
OverflowError: long int too large to convert to int

I did two changes just to test out:

-    outbuflen = len(outbuf)
+   outbuflen = outbuf.__len__()

and

- sz = len(buf)
+ sz = buf.__len__()

Then we end up with:

2013-11-15 15:05:47.369 ERROR [waitress] uncaptured python exception, 
closing channel <waitress.channel.HTTPChannel connected 127.0.0.1:33700 at 
0xc1a9a0c> (<type 'exceptions.OverflowError'>:long int too large to convert 
to int [/usr/lib/python2.6/asyncore.py|write|86] 
[/usr/lib/python2.6/asyncore.py|handle_write_event|450] 
[/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py| 
handle_write|124] 
[/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py| 
total_outbufs_len|92]) 
2013-11-15 15:05:47.369 ERROR [waitress] Unknown exception while trying to 
close outbuf 
Traceback (most recent call last): 
  File 
"/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/channel.py", 
 line 273, in handle_close 
    outbuf._close() 
  File 
"/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/buffers.py", 
 line 295, in _close 
    buf._close() 
  File 
"/data/rhodecodeProduktion/rhodecode-venv/lib/python2.6/site-packages/waitress/buffers.py", 
 line 110, in _close 
    self.file.close() 
IOError: close() called during concurrent operation on the same file 
object.

@marcinkuzminski

This comment has been minimized.

Show comment
Hide comment
@marcinkuzminski

marcinkuzminski Nov 15, 2013

Member

I did this test to mimic the issue on append():

    def test_create_buffer_with_remain_more_than_max_int(self):
        from waitress.buffers import OverflowableBuffer, TempfileBasedBuffer

        class IntOverflowBuffer(TempfileBasedBuffer):

            @property
            def remain(self):
                # maxint + 1
                return 0x7fffffffffffffff + 1

            @remain.setter
            def remain(self, data):
                pass

        inst = self._makeOne()
        inst.buf = OverflowableBuffer(False)
        # mimick _set_large_buffer, method
        inst.buf.buf = IntOverflowBuffer()
        inst.overflowed = True

        # append data to already huge buffer
        inst.buf.append(b'x')

But i have no idea how to reliable test the handle_write() and flush() issue.

Member

marcinkuzminski commented Nov 15, 2013

I did this test to mimic the issue on append():

    def test_create_buffer_with_remain_more_than_max_int(self):
        from waitress.buffers import OverflowableBuffer, TempfileBasedBuffer

        class IntOverflowBuffer(TempfileBasedBuffer):

            @property
            def remain(self):
                # maxint + 1
                return 0x7fffffffffffffff + 1

            @remain.setter
            def remain(self, data):
                pass

        inst = self._makeOne()
        inst.buf = OverflowableBuffer(False)
        # mimick _set_large_buffer, method
        inst.buf.buf = IntOverflowBuffer()
        inst.overflowed = True

        # append data to already huge buffer
        inst.buf.append(b'x')

But i have no idea how to reliable test the handle_write() and flush() issue.

@marcinkuzminski

This comment has been minimized.

Show comment
Hide comment
@marcinkuzminski

marcinkuzminski Nov 20, 2013

Member

@tseaver any idea how to test the handle_write() ?

Member

marcinkuzminski commented Nov 20, 2013

@tseaver any idea how to test the handle_write() ?

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Nov 20, 2013

Member

@marcinkuzminski can you tell whether this error is raised in the situation that @tseaver suggested? He suggested it happens when a ReadOnlyFileBasedBuffer is added to self.outbufs at https://github.com/Pylons/waitress/blob/master/waitress/channel.py#L310 . It'd mean that you were using wsgi.file_wrapper to serve a file, is that true?

Member

mcdonc commented Nov 20, 2013

@marcinkuzminski can you tell whether this error is raised in the situation that @tseaver suggested? He suggested it happens when a ReadOnlyFileBasedBuffer is added to self.outbufs at https://github.com/Pylons/waitress/blob/master/waitress/channel.py#L310 . It'd mean that you were using wsgi.file_wrapper to serve a file, is that true?

@marcinkuzminski

This comment has been minimized.

Show comment
Hide comment
@marcinkuzminski

marcinkuzminski Nov 20, 2013

Member

@mcdonc unfortunately i don't have access to this information (it's a machine of one of our clients)

This error doesn't occur when serving files, it's a clone operation of a huge (around 6GB) mercurial repository.
infact when i checked on creating the Mercurial wsgi app i've got:

'wsgi.file_wrapper': <class 'waitress.buffers.ReadOnlyFileBasedBuffer'>,

in th environ

Member

marcinkuzminski commented Nov 20, 2013

@mcdonc unfortunately i don't have access to this information (it's a machine of one of our clients)

This error doesn't occur when serving files, it's a clone operation of a huge (around 6GB) mercurial repository.
infact when i checked on creating the Mercurial wsgi app i've got:

'wsgi.file_wrapper': <class 'waitress.buffers.ReadOnlyFileBasedBuffer'>,

in th environ

mcdonc added a commit that referenced this issue Nov 21, 2013

- Fix some cases where the creation of extremely large output buffers…
… (greater

  than 2GB, suspected to be buffers added via ``wsgi.file_wrapper``) might
  cause an OverflowError on Python 2.  See
  #47.

See #47
@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Nov 21, 2013

Member

The presence of 'wsgi.file_wrapper' in the environ doesn't really mean much; anyone who is using waitress will have this in the environ.

Your test demonstrates that the append method of an OverflowableBuffer should probably use __len__() rather than len(). In addition its prune() method should do the same. Also, your first traceback indicates that the _flush_some() method of the HTTPChannel should be using outbuf.__len__ rather than len(outbuf). And finally, your last traceback (the one that seems to emanate from _close()) indicates that the HTTPChannel's total_outbufs_len() method should also use b.__len__() instead of len(b).

I've checked in these fixes but I'm not totally confident that my commit will fix all of the OverflowError cases; hopefully you can test the waitress master and let us know if you see any more of these in production.

Member

mcdonc commented Nov 21, 2013

The presence of 'wsgi.file_wrapper' in the environ doesn't really mean much; anyone who is using waitress will have this in the environ.

Your test demonstrates that the append method of an OverflowableBuffer should probably use __len__() rather than len(). In addition its prune() method should do the same. Also, your first traceback indicates that the _flush_some() method of the HTTPChannel should be using outbuf.__len__ rather than len(outbuf). And finally, your last traceback (the one that seems to emanate from _close()) indicates that the HTTPChannel's total_outbufs_len() method should also use b.__len__() instead of len(b).

I've checked in these fixes but I'm not totally confident that my commit will fix all of the OverflowError cases; hopefully you can test the waitress master and let us know if you see any more of these in production.

@marcinkuzminski

This comment has been minimized.

Show comment
Hide comment
@marcinkuzminski

marcinkuzminski Nov 21, 2013

Member

That was my first reaction to do this (fix len calls), but did you seen what error it triggered ?

Member

marcinkuzminski commented Nov 21, 2013

That was my first reaction to do this (fix len calls), but did you seen what error it triggered ?

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Nov 21, 2013

Member

Yes. See my explanation above wrt "total_outbufs_len". I fixed more than you did.

Member

mcdonc commented Nov 21, 2013

Yes. See my explanation above wrt "total_outbufs_len". I fixed more than you did.

@marcinkuzminski

This comment has been minimized.

Show comment
Hide comment
@marcinkuzminski

marcinkuzminski Nov 21, 2013

Member

Great, thanks for the very detailed explanation and fixes, i'll push to test latest master on troublesome repositories. I'll share some feedback when we get it.

Member

marcinkuzminski commented Nov 21, 2013

Great, thanks for the very detailed explanation and fixes, i'll push to test latest master on troublesome repositories. I'll share some feedback when we get it.

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Jul 14, 2014

Member

@marcinkuzminski I'm closing this issue, as I'm going to assume what I did fixed your production errors. If that's not the case, please reopen.

Member

mcdonc commented Jul 14, 2014

@marcinkuzminski I'm closing this issue, as I'm going to assume what I did fixed your production errors. If that's not the case, please reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment