Bug 312320 - kio sftp file corruption for certain file sizes
Summary: kio sftp file corruption for certain file sizes
Alias: None
Product: kio
Classification: Unclassified
Component: sftp (show other bugs)
Version: 4.9.4
Platform: Other Linux
: NOR critical (vote)
Target Milestone: ---
Assignee: Andreas Schneider
: 315841 (view as bug list)
Depends on:
Reported: 2012-12-28 21:12 UTC by Peter Klotz
Modified: 2013-02-28 11:46 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.10.1

Patch that fixes the problem (637 bytes, patch)
2012-12-29 08:02 UTC, Peter Klotz
Patch that fixes the underlying cause of the problem which is a "use after delete" (938 bytes, patch)
2013-01-05 19:56 UTC, Peter Klotz

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Klotz 2012-12-28 21:12:39 UTC
Files that are a multiple of 60KiB (61440 bytes) are corrupted when downloading them from a server via sftp. The problem can be observed in Dolphin 2.1 (KDE 4.9.4, Arch Linux). 

All files end up with an extra 60KiB chunk at the end that mostly consists of zeros. The leading part of the file is correct.

I tested against servers running under cygwin 1.7 and Red Hat Enterprise Linux 6. Both give the same results. Copying the files with rsync over ssh works as expected.

Reproducible: Always

Steps to Reproduce:
1. Create a file whose size is a multiple of 61440 bytes on the server, e.g. "dd if=/dev/urandom of=60kB bs=60k count=1"
2. Copy the file in Dolphin using protocol "sftp" to the local machine

Actual Results:  
The local file has 122880 bytes instead of 61440.

Expected Results:  
The file should have 61440 bytes and the content should match the original file.

The problem does not occur when using protocol "fish", therefore I assume it is not a Dolphin but a kio sftp problem.
Comment 1 Peter Klotz 2012-12-29 08:02:12 UTC
Created attachment 76075 [details]
Patch that fixes the problem

If the transfer offset equals the file size no more requests have to be enqueued.
Comment 2 Rolf Eike Beer 2013-01-02 14:35:04 UTC
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.
Comment 3 Michael Pyne 2013-01-03 04:55:12 UTC
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.
Comment 4 Michael Pyne 2013-01-05 04:10:59 UTC
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.

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

Comment 5 Peter Klotz 2013-01-05 19:56:54 UTC
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)

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
Comment 6 Michael Pyne 2013-01-06 19:21:54 UTC
@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 7 Andreas Schneider 2013-01-09 16:47:57 UTC
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.
Comment 8 Michael Pyne 2013-01-11 00:28:40 UTC
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).
Comment 9 Jan Lepper 2013-02-06 16:10:03 UTC
Changing severity to critical because of data corruption.
Comment 10 Jan Lepper 2013-02-06 16:10:35 UTC
*** Bug 313052 has been marked as a duplicate of this bug. ***
Comment 11 Michael Pyne 2013-02-09 22:48:10 UTC
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.

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

Comment 12 Andreas Schneider 2013-02-28 11:46:18 UTC
*** Bug 315841 has been marked as a duplicate of this bug. ***