Bug 342391 - kio_sftp.so corrupts files when reading
Summary: kio_sftp.so corrupts files when reading
Alias: None
Product: kio
Classification: Frameworks and Libraries
Component: sftp (show other bugs)
Version: 4.14.1
Platform: Fedora RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Andreas Schneider
Depends on:
Reported: 2015-01-01 15:55 UTC by Albert Flügel
Modified: 2015-01-08 10:04 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:

Patch to fix this issue with a Redhat-like build system (1.10 KB, patch)
2015-01-07 20:11 UTC, Albert Flügel

Note You need to log in before you can comment on or make changes to this bug.
Description Albert Flügel 2015-01-01 15:55:04 UTC
Reading a file over sftp in dolphin corrupts it. More exactly: in the end it is nearly double as long and has repeating parts. The bug is in kio_sftp.so

Version-Release number of selected component: 4.14.3-2.fc20
(4.14.3 is not offered in the menu above)

The temporary file created from the download in /var/tmp/kdecache-<username>/krun/... is larger than the original. When examining it, one can find, that parts repeat inside.
The problem has to do with the queued handling of read requests in kioslave/sftp/kio_sftp.cpp . In sftpProtocol::GetRequest::readChunks i see, that if not enough bytes have been read, the request is re-issued, what might not be the appropriate action. Particularly what i see happening: First 32768 bytes are read. 61440 had been requested. So the request is re-issued. Next 28672 bytes are read. Then 32768 are read, which start with the same stuff like the 28672 bytes read before. So something is quite badly wrong here.

I haven't yet fully understood how to fix this. When i find it i'll post, but i don't have arbitrary time and i guess the maintainer is more familiar with the code.

I think the assumtion in sftpProtocol::GetRequest::readChunks, that it is an error, when sftp_async_read returns less bytes than requested, is not appropriate. Especially when reading asynchronously and e.g. over a not so fast connection (what is the case here), i'd consider it normal, that i have to issue several read requests. I'll try to examine this in more detail or patch, but i can't promise, that i'll get it done in the next time.

Reproducible: Always

Steps to Reproduce:
1. Install sshd on your android smartphone. Probably it also happens with other servers offering sftp
2. Connect in dolphin e.g. right mouse key on the left pane, select "Add Entry ..."
3. Enter sth. like sftp://<usename-on-smartphone>:<password-on-smartphone>@<your-smartphone-network-name>:<port>/ (ignore your personal security concerns regarding the password in the URL for a moment, later you can use RSA or other pubkey authentication or whatever)
4. Navigate to a JPEG image
5. open it with any picture viewer

Actual Results:  
The accessed file is corrupt. For more details please see in the Details section

Expected Results:  
The accessed file is downloaded as is and can be used

It does not happen with Gnome's nautilus (who is calling ssh as subprogram and does not use libssl).
I switched on debug mode like explained in https://techbase.kde.org/Development/Tutorials/Debugging/Debugging_IOSlaves/Debugging_kio_sftp#SFTP_Logging. The compressed log can be downloaded here: http://www.muc.de/~af/kio_sftp.log.xz
Comment 1 Albert Flügel 2015-01-02 12:55:20 UTC
I found a fix. Please check, if this is correct. At least here it fixes the problem. I replaced daeling with mFile->offset with a sftp_seek64 . Imo the mFile structure should be considered opaque, so fiddling with it's internals may lead to inconsistencies. The respective section in sftpProtocol::GetRequest::readChunks now reads:

    if (bytesread < request.expectedLength) {
      // If less data is read than expected - requeue the request
      data.resize(data.size() - (request.expectedLength - bytesread));

      // Save current file offset
    //  uint64_t oldOffset = mFile->offset;
    //  mFile->offset = request.startOffset + bytesread;

      // Modify current request
      request.expectedLength -= bytesread;
      request.startOffset += bytesread;

      if (sftp_seek64(mFile, request.startOffset) < 0) {
        // Failed to continue reading
        return -1;

      request.id = sftp_async_read_begin(mFile, request.expectedLength);

      // Restore the file offset
    //  mFile->offset = oldOffset;

      if (request.id < 0) {
        // Failed to dispatch rerequest
        return -1;

      return totalRead;
Comment 2 Andreas Schneider 2015-01-06 09:55:03 UTC
I've just tested it and it works without any issues for me. How big is the file you try to open?

I think the code should use sftp_tell64() and sftp_seek64() here.
Comment 3 Andreas Schneider 2015-01-06 09:59:15 UTC
 kioslave/sftp/kio_sftp.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kioslave/sftp/kio_sftp.cpp b/kioslave/sftp/kio_sftp.cpp
index 21cffac..7e9065c 100644
--- a/kioslave/sftp/kio_sftp.cpp
+++ b/kioslave/sftp/kio_sftp.cpp
@@ -2165,16 +2165,16 @@ int sftpProtocol::GetRequest::readChunks(QByteArray &data) {
       data.resize(data.size() - (request.expectedLength - bytesread));
       // Save current file offset
-      uint64_t oldOffset = mFile->offset;
-      mFile->offset = request.startOffset + bytesread;
+      uint64_t oldOffset = sftp_tell64(mFile);
+      sftp_seek64(request.startOffset + bytesread);
       // Modify current request
       request.expectedLength = request.expectedLength - bytesread;
-      request.startOffset = mFile->offset;
+      request.startOffset = sftp_tell64(mFile);
       request.id = sftp_async_read_begin(mFile, request.expectedLength);
       // Restore the file offset
-      mFile->offset = oldOffset;
+      sftp_seek64(mFile, oldOffset);
       if (request.id < 0) {
         // Failed to dispatch rerequest
Comment 4 Albert Flügel 2015-01-07 08:39:43 UTC
I tried with files with between about 30 kB and 4 MB.
Frankly i don't understand what's the use of all the 2nd sftp_tell64 and seek calls.
If the seek succeeds, this is the new offset. If it fails, there's hardly a chance to continue.
The sftp_async_read_begin might not change the offset and to get the offset after the next read, the tell should be done after sftp_async_read . From the sftp documentation i do not get a real clue, when the read pointer is updated and in which of the functions, so i would refrain from relying on it.
However. My 2c
Comment 5 Andreas Schneider 2015-01-07 10:20:47 UTC
I guess your changes from comment #1 are probably correct.
Comment 6 Andreas Schneider 2015-01-07 10:31:29 UTC
Can you create a git format patch?
Comment 7 Albert Flügel 2015-01-07 20:11:50 UTC
Created attachment 90277 [details]
Patch to fix this issue with a Redhat-like build system

my 2c
Comment 8 Albert Flügel 2015-01-07 20:12:33 UTC
Never created git format-patch stuff, sorry.
The attached patch works for redhat like systems with these lines in the spec file:
Patch110: kde-runtime-4.14.3-sftp-offset.patch
%patch110 -p1 -b .sftp-offset
Comment 9 Andreas Schneider 2015-01-08 10:04:22 UTC
Thanks, I've pushed the fix to master and KDE/4.14.