Bug 312320

Created attachment 76075 [details]
Patch that fixes the problem

If the transfer offset equals the file size no more requests have to be enqueued.

I'm unsure about the fix. I would have expected the next time something is read from the network it would return with size 0 to indicate the file has not grown meanwhile. Again, I have no idea about how _this_ code works, but that is the way such network protocols usually operate. At least those that I have used.

I agree at a halfway in-depth look at the code that it should be handling the condition properly (given that libssh returns 0 bytes read as would make sense), but on the other hand I can also confirm the bug (though I haven't tested the proposed patch). Notably, the 61440 byte figure corresponds exactly with the expected request size (defined to be 60 * 1024).

It is possible that trying to call the read function after the initial end-of-stream from libssh might confuse the issue, which the reporter's patch fixes. I'm still trying to sort out why the only function that resizes the QByteArray eventually written to disk somehow ends up forgetting to subtract back out the 61440 bytes it adds in to start with, but the patched conditional test is still correct. So if it fixes the bug I would recommend committing.

I would do it myself but it's very late here (and I'm very tired) and won't have the chance to revert if it breaks something... but I can commit tomorrow night if no one else does.

Git commit 7f87a968f622d95b5279fece58a1717d52ba23b9 by Michael Pyne.
Committed on 05/01/2013 at 04:25.
Pushed by mpyne into branch 'KDE/4.10'.

kio_sftp: Use proper boundary when queueing requests.

This fixes an off-by-one error when deciding whether the sftp_get code
path needs to make another request or not. This appears to be involved
in a data corruption bug (bug 312320) where file sizes that are a
multiple of the request buffer size (60*1024) end up getting an extra
full buffer of gibberish appended. E.g. a 61440-size file gets
downloaded to a 122880-sized file.

The bug report and proposed fix were both provided by Peter Klotz. I do
not yet understand at this point why this off-by-one error would cause
the extra packet to be appended, but I can confirm that the fix works.
It's at least easy enough to prove that a valid offset will not be == to
the file size (compare with the test on line 1151, for instance), and I
can confirm the fix seems to work.

It still doesn't make sense to me that the bug would be here though;
reading from an sftp channel that's already at EOF just returns 0 bytes,
and our code appears to handle that condition properly. In fact, the
check that is being corrected happens only *after* unconditionally
queueing up a first request packet, so you'd think the second EOF
response wouldn't even be an issue. Yet here we are.
FIXED-IN:4.10

M  +1    -1    kioslave/sftp/kio_sftp.cpp

http://commits.kde.org/kde-runtime/7f87a968f622d95b5279fece58a1717d52ba23b9

Created attachment 76216 [details]
Patch that fixes the underlying cause of the problem which is a "use after delete"

@Michael: After debugging the kio_sftp module I finally understand what happens.

Code extracted from kio_sftp.cpp:
--------------------------
    sftpProtocol::GetRequest::Request &request = pendingRequests.head();
    ...
    if (bytesread == 0 || bytesread == SSH_AGAIN) {
      if (bytesread == 0) {
        pendingRequests.dequeue();  <--- here reference "request" loses its instance
      } else {
        // Decrease maximum pending requests as we did not receive data fast enough
        mMaxPendingRequests = qMax(1, mMaxPendingRequests / 2);
      }
      // Done reading or timeout
      data.resize(data.size() - request.expectedLength); <--- An access to "request.expectedLength" results in garbage (in my case 0 all the time)
      break;
--------------------------

So what happens is that data.resize() turns into a no-op since request.expectedLength is zero instead of 61440. The intended correction of the buffer size never takes place. As a result sftpProtocol::sftpGet() writes data that contains a trailing buffer of garbage.

The problem only occurs for file sizes that are a multiple of the buffer size since this is the only case where a single iteration in sftpProtocol::sftpGet() results in data and hits the bytesread==0 case in sftpProtocol::GetRequest::readChunks(). This is due to the extra request that was dispatched without the first patch. All other cases perform another iteration in sftpProtocol::sftpGet() whose sole purpose is to detect server side EOF and return without data.

So my suggestion is to keep the original patch since it removes a request that will never retrieve data and additionally apply this patch since it fixes the real problem.

I tested:
 * kde-runtime-4.9.4-sftp.patch alone works
 * kde-runtime-4.9.4-sftpUseAfterDelete.patch alone works
 * kde-runtime-4.9.4-sftp.patch and kde-runtime-4.9.4-sftpUseAfterDelete.patch together work

@Peter: Great work troubleshooting, I agree with your diagnosis and with the need to correct the other latent bug we have. Given that we're already working around it in the bug fix pushed to 4.10 I'm not eager to apply to 4.10 immediately given that it's frozen except for "showstopper" bugs (I already bent the rule on that one ;). But we'll definitely want to apply for 4.10.1 and to master.

I've reopened the bug so that I (hopefully) don't forget to apply the patch when 4.10 is unfrozen. I wonder if there are other problem areas like this with the request queue?

P.S. Have you considered applying for a KDE Developer account or were you simply trying to unbreak kio_sftp and step out? Given that we'll be blocked waiting for the freeze to end you have time to get an account and apply yourself. ;)

Comment on attachment 76216 [details]
Patch that fixes the underlying cause of the problem which is a "use after delete"

So is someone already pushing that already to the repos or should I do that. Patch looks fine.

Andreas, I will push once 4.10 has been tagged (we are trying to commit bugfixes only to stable branches so that we can use git-merge on master instead of git-cherry-pick).

Changing severity to critical because of data corruption.

*** Bug 313052 has been marked as a duplicate of this bug. ***

Git commit 03597e39b7c107a3dacf1f5f682bd56cd216ddb1 by Michael Pyne.
Committed on 09/02/2013 at 23:42.
Pushed by mpyne into branch 'KDE/4.10'.

kio_sftp: Fix use-after-free leading to data corruption.

Apply patch from Peter Klotz to correct a use-after-free situation in
kio_sftp, which causes an extra buffer of 60 kbytes to be written out
when the file size was itself a multiple of 60 kbytes.

See the bug entry for a more detailed description of the cause. A
workaround for this issue made it to KDE 4.10, this is the proper fix.

Thanks to Peter for reporting, debugging, and fixing the issue and from
everyone who reviewed the patch.
FIXED-IN:4.10.1

M  +4    -3    kioslave/sftp/kio_sftp.cpp

http://commits.kde.org/kde-runtime/03597e39b7c107a3dacf1f5f682bd56cd216ddb1

*** Bug 315841 has been marked as a duplicate of this bug. ***