OverflowError: Python int too large to convert to C long #353

Closed
fz0dbx opened this Issue Jul 8, 2014 · 27 comments

Projects

None yet

9 participants

@fz0dbx
fz0dbx commented Jul 8, 2014

I am getting the following stack trace on a 32bit Debian jessie computer:

File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 155, in listdir
return [f.filename for f in self.listdir_attr(path)]
File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 193, in listdir_attr
attr = SFTPAttributes._from_msg(msg, filename, longname)
File "/usr/lib/python2.7/dist-packages/paramiko/sftp_attr.py", line 91, in _from_msg
attr._unpack(msg)
File "/usr/lib/python2.7/dist-packages/paramiko/sftp_attr.py", line 104, in _unpack
self.st_uid = msg.get_int()
File "/usr/lib/python2.7/dist-packages/paramiko/message.py", line 140, in get_int
return util.inflate_long(self.get_binary())
File "/usr/lib/python2.7/dist-packages/paramiko/message.py", line 198, in get_binary
return self.get_bytes(self.get_size())
File "/usr/lib/python2.7/dist-packages/paramiko/message.py", line 108, in get_bytes
b = self.packet.read(n)
OverflowError: Python int too large to convert to C long

It does not happen on a 64bit machine.

As you can see, all that I am doing is a listdir.
When I run in a debugger and print the value of n, I get 4294967039L
Seems like a bug in Paramiko.

@mikegedelman

I'm having the same issue on Google App Engine. Works locally on my machine, but not when deployed. Oddly, it only happens for some hosts.

@jbouse
jbouse commented Sep 22, 2014

I actually don't have any 32bit systems running around to test the Debian package on.

@ksaylor11

I'm experiencing the same problems. Does anyone have any hunches?

@dboreham
dboreham commented Jan 6, 2015

I ran into this issue today, found this bug report and did some investigation. As far as I can see the problem is in some (to me at least) quite odd code in message.py. In particular the function Message.get_int() treats the case where the leading byte is 0xff differently (and to my eye incorrectly). This in turn leads to the > 32-bit value for the uint32 being decoded, causing the Python integer overflow exception. That code path was exercised while decoding the sftp channel open response from the server because the server I connected to sent 0xffffffff as the window size. I coded a quick fix that for me at least makes my sftp work. I don't know enough about the ssh protocol encoding to know if I broke other stuff though. I also noticed in passing that there are two instances of the method get_size() in Message. Surely that's not correct? If someone familiar with the code could let me know if I'm on the right track (or not), I can submit a pr with my fix. Curiously, I worked with Robey many years ago -- had no idea he had written this code!

@dboreham
dboreham commented Jan 6, 2015

fwiw it looks like the strange get_int() code was introduced in this change : 0e4ce37
and the double copy of get_size() came from a (very large) merge:
e7f41de

@dboreham
dboreham commented Jan 6, 2015

btw the fix I tested turns out to be the version of get_int() prior to 0e4ce37

@dboreham
dboreham commented Jan 8, 2015

Update on this: I found when I tested on a 64-bit machine that although the specific problem reported here does not show up (presumably because the bogus value returned by get_int doesn't throw an integer overflow exception), the underlying bug is still present. I see my sftp puts stall forever when the uploaded file is more than trivially small. On a hunch that the channel window size was incorrect due to the get_int() bug, I changed the code on the 64-bit machine. Lo and behold, the sftp put problem vanished.

@ksaylor11

so the issue is with the channel window size?

@dboreham
dboreham commented Jan 8, 2015

The issue is with the low-level protocol decode -- anything that's a uint32 can be incorrectly decoded, or worse cause an integer overflow exception, if the MSB has the value -xff. The window size is sent as a uint32.

@dboreham
dboreham commented Jan 8, 2015

This is the fix that works for me: #475

@dboreham
dboreham commented Jan 9, 2015

Note that my fix fails message test #4 but afaics that's because the test was written such that it passed the existing incorrect code for get_int() (see comment here: #475 (comment))

@expandrive

We were experiencing this with some SFTP servers, I can confirm #475 fixes it.

@bitprophet
Member

I see my sftp puts stall forever when the uploaded file is more than trivially small.

I cannot reproduce this on a 64bit system with a 100MB file, so unless that falls under your definition of trivial, I apparently need other guidance on a good way to trigger this bug. If it feels like any 32 bit system is likely to encounter it, I can try installing a 32bit guest on a VM sometime.

It's unclear to me why @scottkmaxwell made the change (hopefully he will reply; it was added during addition of Python 3 support which unfortunately introduced widespread changes) but until we know why it appeared to be necessary, I worry that simply reverting it might trigger bugs elsewhere.

@scottkmaxwell
Contributor

If I recall correctly, the reason for the change to add_int and get_int was because the _add method automatically switched between int and an adaptive int/ mpint based on whether or not the number was and int or a long. There is no distinction between the two in Python 3 so this caused trouble. I moved the location and type of the check, but I see now that my solution conflated cases where the byte size was critical with cases where only the ability to transmit the proper value mattered. I believe I added get_size and add_size to handle explicit 32-bit values, but I didn't catch all of the places where that was necessary. Maybe we should be really explicit and have add_int32, add_int64 and add_adaptive_int.

Simply reverting to the older add_int would almost certainly break some things on Py3.

@scottkmaxwell
Contributor

I think the best solution will be having add_int and get_int perform their old function and to create a new add_adaptive_int and get_adaptive_int for use by the add method. Looking through the code, I do not see anywhere outside of the tests that actually makes use of the adaptive int encoding. Maybe that adaptive code is some legacy piece that is no longer in use.

I just about have a pull request ready. It is only slightly more complex than #475.

@bitprophet
Member

Thanks a bundle for the prompt and thorough response, @scottkmaxwell!

If at least a couple of @fz0dbx @mikegedelman @jbouse @ksaylor11 @dboreham can confirm that Scott's PR still fixes the issue for them, that'd be awesome. (Still interested in reproducing the core bug locally, though, if anybody's got a foolproof method.)

I can confirm at least that running Fabric & Paramiko's test suites with his branch under Python 2.6.9, still passes, and Paramiko's test suite under Python 3.4.2, ditto.

@scottkmaxwell
Contributor

Yes, help validating would be great. I do not have a 32-bit system either. It looks like the bug would have caused bogus attribute values to come across on listdir for certain attribute values, even on 64-bit. No exception, just wrong values.

@dboreham

I can validate the fix easily. Don't have time right now but I can probably do it later tonight.

@dboreham

fwiw I did see the bug on both 32-bit and 64-bit systems (manifesting in different ways though).

@diogo-dsco

SFTPClient.listdir() (sftp_client.py) returns a wrong result, if the directory contains entries with group id >= 0xff000000. This is caused by a change from paramiko 1.12.4 to 1.13.0 in Message.get_int() (message.py):

Message.get_int() in paramiko 1.12.4:

def get_int(self):
    """
    Fetch an int from the stream.

    :return: a 32-bit unsigned `int`.
    """
    return struct.unpack('>I', self.get_bytes(4))[0]

Message.get_int() in paramiko 1.13.0 (also in 1.15.2):

def get_int(self):
    """
    Fetch an int from the stream.

    :return: a 32-bit unsigned `int`.
    """
    byte = self.get_bytes(1)
    if byte == max_byte:
        return util.inflate_long(self.get_binary())
    byte += self.get_bytes(3)
    return struct.unpack('>I', byte)[0]

Note: max_byte = '\xff'

The problem can be demonstrated with the following directory (group “group” was created via “sudo groupadd -K GID_MAX=4294967295 -g 4294967294 group”):

% ls -l /tmp/dir
total 0
-rw-r--r-- 1 user group 0 Feb 9 15:01 a
-rw-r--r-- 1 user group 0 Feb 9 15:01 b
-rw-r--r-- 1 user group 0 Feb 9 15:01 c

% ls -ln /tmp/dir
total 0
-rw-r--r-- 1 40002 4294967294 0 Feb 9 15:01 a
-rw-r--r-- 1 40002 4294967294 0 Feb 9 15:01 b
-rw-r--r-- 1 40002 4294967294 0 Feb 9 15:01 c

using the following demo program:

--- BEGIN
import paramiko

paramiko.util.log_to_file("/tmp/paramiko.log")

transport = paramiko.Transport(("localhost", 22))
transport.connect(username="user", password="password")

sftp = paramiko.SFTPClient.from_transport(transport)

print sftp.listdir("/tmp/dir")

sftp.close()
transport.close()
--- END

If the following additional log for msg is added in SFTPClient.listdir_attr() (sftp_client.py):

        try:
            t, msg = self._request(CMD_READDIR, handle)
            self._log(DEBUG, 'msg=%r' % msg)
        except EOFError:

then /tmp/paramiko.log contains the following output:

...
INF [20150209-15:21:01.572] thr=2 paramiko.transport.sftp: [chan 0] Opened sftp connection (server version 3)
DEB [20150209-15:21:01.572] thr=2 paramiko.transport.sftp: [chan 0] listdir('/tmp/dir')
DEB [20150209-15:21:01.916] thr=2 paramiko.transport.sftp: [chan 0] msg=paramiko.Message('\x00\x00\x00\x02\x00\x00\x00\x05\x00\x00\x00\x01.\x00\x00\x009drwxr-xr-x 2 fsmr3rt public 30 Feb 9 15:01 .\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x00\x1e\x03\xa9i\x06\x00\x00'"\x00\x00A\xedT\xd8\xbd\xacT\xd8\xbd\xa8\x00\x00\x00\x02..\x00\x00\x00:drwxrwxrwt 28 root root 4096 Feb 9 15:21 ..\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00C\xffT\xd8\xc2\x12T\xd8\xc2M\x00\x00\x00\x01a\x00\x00\x009-rw-r--r-- 1 user group 0 Feb 9 15:01 a\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x9cB\xff\xff\xff\xfe\x00\x00\x81\xa4T\xd8\xbd\xa8T\xd8\xbd\xa8\x00\x00\x00\x01b\x00\x00\x009-rw-r--r-- 1 user group 0 Feb 9 15:01 b\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x9cB\xff\xff\xff\xfe\x00\x00\x81\xa4T\xd8\xbd\xa8T\xd8\xbd\xa8\x00\x00\x00\x01c\x00\x00\x009-rw-r--r-- 1 user group 0 Feb 9 15:01 c\x00\x00\x00\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x9cB\xff\xff\xff\xfe\x00\x00\x81\xa4T\xd8\xbd\xa8T\xd8\xbd\xa8')
INF [20150209-15:21:01.918] thr=2 paramiko.transport.sftp: [chan 0] sftp session closed.
DEB [20150209-15:21:01.918] thr=2 paramiko.transport: [chan 0] EOF sent (0)
DEB [20150209-15:21:01.919] thr=1 paramiko.transport: EOF in transport thread

The demo program will print the following output:

[u'a', u'', u'']

The call chain is:

  • SFTPClient.listdir() [sftp_client.py]

  • SFTPClient.listdir_attr() [sftp_client.py]

  • SFTPAttributes._from_msg() [sftp_attr.py]

  • SFTPAttributes._unpack() [sftp_attr.py]:

    def _unpack(self, msg):
    self._flags = msg.get_int()
    if self._flags & self.FLAG_SIZE:
    self.st_size = msg.get_int64()
    if self._flags & self.FLAG_UIDGID:
    self.st_uid = msg.get_int()
    self.st_gid = msg.get_int()
    if self._flags & self.FLAG_PERMISSIONS:
    self.st_mode = msg.get_int()
    if self._flags & self.FLAG_AMTIME:
    self.st_atime = msg.get_int()
    self.st_mtime = msg.get_int()
    if self._flags & self.FLAG_EXTENDED:
    count = msg.get_int()
    for i in range(count):
    self.attr[msg.get_string()] = msg.get_string()

self.st_gid (and other fields for the attributes of a directory entry) should be filled by a function that returns the next 4 bytes as an 32-bit integer and does no special handling, if the first of these bytes is 0xff.

Additional remark about message.py: Method Message.get_size() is defined twice:

def get_size(self):
    """
    Fetch an int from the stream.

    @return: a 32-bit unsigned integer.
    @rtype: int
    """
    byte = self.get_bytes(1)
    if byte == max_byte:
        return util.inflate_long(self.get_binary())
    byte += self.get_bytes(3)
    return struct.unpack('>I', byte)[0]

def get_size(self):
    """
    Fetch an int from the stream.

    @return: a 32-bit unsigned integer.
    @rtype: int
    """
    return struct.unpack('>I', self.get_bytes(4))[0]
@scottkmaxwell
Contributor

Should all be fixed in #482

@bitprophet
Member

One of @diogo-dsco's colleagues actually mailed me about the above over the weekend, I replied to them asking if they would be able to test out #482 :)

@diogo-dsco

We have tested #482 and it's working

@bitprophet
Member

@dboreham have you had a chance to try the fix yet? :)

@dboreham

Apologies: got distracted by a few squirrels in trees. However I was finally able to test today and confirmed that #482 fixes the bug in my reproduction scenario. I backed out by original fix (re-installed the stock paramiko with pip uninstall/install), confirmed the original problem then returned (throws integer overflow exception), then applied #482 , re-tested and observed the test program worked correctly and without exceptions being thrown.

@bitprophet
Member

Awesome, thanks @dboreham. Adding to bugfix release milestone, just need to make a changelog & figure out how far back I can port this cleanly (since the breakage was in the 1.13 python3 compat release, hopefully that far back.)

@bitprophet bitprophet added this to the 1.13.4 / 1.14.3 / 1.15.3 milestone Feb 27, 2015
@bitprophet
Member

Looks like #482 was based off of 1.15 (or pre-1.15 master) so I'll go with that as the base for now.

Ran paramiko & fabric tests against current state of #482, they were happy.

Merged up to latest 1.15 branch, ditto.

Making changelog and merging.

@bitprophet bitprophet added a commit that closed this issue Sep 30, 2015
@bitprophet bitprophet Changelog closes #353 aef405c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment