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
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; }
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.
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
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
I guess your changes from comment #1 are probably correct.
Can you create a git format patch?
Created attachment 90277 [details] Patch to fix this issue with a Redhat-like build system my 2c
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
Thanks, I've pushed the fix to master and KDE/4.14.