sftp: fix seekPos + file resuming when part file is of size 11
ClosedPublic

Authored by sitter on Mar 5 2020, 2:56 PM.

Details

Summary

previously seekPos would loop over offset==EAGAIN. the returned off_t of
seek is not an error, but the offset or -1. this incorrect handling
of the return value resulted in attempting to seek a file of the size 11
to get stuck in an infinite loop as EAGAIN==11 and so the loop would
always be true. any other file size would have been fine, so the impact
of this is actually super small.

also sync up the expectation and handling a bit more between copy and put
scenarios.
specifically we always seek to the size we (think) the part file has,
instead of letting the libc determine the end. this is in part so
we can simply do an offset==size comparison to check if the seek worked
to the end we expected it to.

the seekPos() helper was removed as it now serves no purpose over calling
lseek directly.

BUG: 417645
FIXED-IN: 20.04

Test Plan
  • create file
  • split -b 11 file to get a segment exactly EAGAIN size
  • copy first segment to some other dir as file.part
  • open sftp to other dir and copy file there
  • copy doesn't get stuck, the file.part is removed, and the resulting file is same as input
  • vice versa copy from sftp to local

Diff Detail

Repository
R320 KIO Extras
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Mar 5 2020, 2:56 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptMar 5 2020, 2:56 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Mar 5 2020, 2:56 PM
bruns added a subscriber: bruns.Mar 5 2020, 3:42 PM
bruns added inline comments.
sftp/kio_sftp.cpp
1687

You should save errno immediately after QT_LSEEK, otherwise any function may overwrite it. See man 3 errno:

A common mistake is to do

if (somecall() == -1) {
    printf("somecall() failed\n");
    if (errno == ...) { ... }
}

where errno no longer needs to have the value it had upon return from somecall() (i.e., it may have been changed by the printf(3)). If the value of errno should be preserved across a library call, it must be saved:

if (somecall() == -1) {
    int errsv = errno;
    printf("somecall() failed\n");
    if (errsv == ...) { ... }
}
1955–1957

dito

bruns added a comment.Mar 5 2020, 4:07 PM

Can you also mention why errno == EGAIN does not have to be handled ("EAGAIN could only happen iff the file where opened with O_NONBLOCK. All other seek errors are fatal.").

sitter added a comment.Mar 6 2020, 5:38 PM

Can you also mention why errno == EGAIN does not have to be handled ("EAGAIN could only happen iff the file where opened with O_NONBLOCK. All other seek errors are fatal.").

I do not understand what that means.

sitter added inline comments.Mar 6 2020, 5:42 PM
sftp/kio_sftp.cpp
1687

Which call between the seek and the debug do you take offense here exactly?

bruns added inline comments.Mar 6 2020, 7:15 PM
sftp/kio_sftp.cpp
1687

qCDebug(), operator<<

bruns added a comment.Mar 6 2020, 7:17 PM

Can you also mention why errno == EGAIN does not have to be handled ("EAGAIN could only happen iff the file where opened with O_NONBLOCK. All other seek errors are fatal.").

I do not understand what that means.

Previously, EAGAIN was handled explicitly (although the implementation was wrong). Why is it fine to not handle it? This belongs in the commit message.

Can you also mention why errno == EGAIN does not have to be handled ("EAGAIN could only happen iff the file where opened with O_NONBLOCK. All other seek errors are fatal.").

I do not understand what that means.

Previously, EAGAIN was handled explicitly (although the implementation was wrong). Why is it fine to not handle it? This belongs in the commit message.

But lseek does not use EAGAIN, does it?
In any case, can't you edit the message to make it say what you think it should say? (if not I am sure sysadmins can give you magic powers)

sftp/kio_sftp.cpp
1687

We do that all over the place.

bruns added inline comments.Mar 9 2020, 1:30 PM
sftp/kio_sftp.cpp
1687

Which part of

A common mistake is to do

did you miss?

sitter added inline comments.Mar 9 2020, 2:34 PM
sftp/kio_sftp.cpp
1687

Oh, I'm sorry. My reading comprehension isn't so good. I am not sure what you are asking of me though. errno access in qdebug streaming is used all over our software and this line here isn't even new. If the errno access wasn't a problem before, why is it now? And what does it have to do with the fix this diff has? Why don't you just do something about the errno problems?

bruns added inline comments.Mar 9 2020, 5:17 PM
sftp/kio_sftp.cpp
1687

Essentially, apply the change from the man page example to the code here:

if (pos != sbPart->size) {
    int errsv = errno;
    qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << sbPart->size << "bytes in source file. Reason given:" << strerror(errsv);
}

As the errno value is now held in a local variable (and thus the global/thread-local errno does not matter), no function called from qCDebug() is able to mess with the value. qCDebug() *may* call all kind of functions (remember, it may log to the terminal, to the system journal, ...) which may fail in a non-fatal way ("if this does not work, try that"), overwriting errno in the course.

Making assumptions about errno can lead to some surprises, and while it may work most of the time it is better to be save than sorry.

sitter added inline comments.Mar 10 2020, 1:25 PM
sftp/kio_sftp.cpp
1687

Can you please make a diff for that?

fish/fish.cpp:        myDebug( << "socketpair failed, error: " << strerror(errno));
fish/fish.cpp:        myDebug( << "fork failed, error: " << strerror(errno));
fish/fish.cpp:        myDebug( << "could not exec! " << strerror(errno));
fish/fish.cpp:            myDebug( << "select failed, rc: " << rc << ", error: " << strerror(errno));
fish/fish.cpp:                myDebug( << "write failed, rc: " << rc << ", error: " << strerror(errno));
fish/fish.cpp:                myDebug( << "read failed, rc: " << rc << ", error: " << strerror(errno));
fish/fish.cpp:                myDebug( << "select failed, rc: " << rc << ", error: " << strerror(errno));
fish/fish.cpp:                    myDebug( << "write failed, rc: " << rc << ", error: " << strerror(errno));
fish/fish.cpp:                    myDebug( << "read failed, rc: " << rc << ", error: " << strerror(errno));
sftp/kio_sftp.cpp:                    qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << sbPart->size << "bytes in source file. Reason given" << strerror(errno);
smb/kio_smb_dir.cpp:            qCDebug(KIO_SMB) << "failed to rename" << dstUrl << "to" << dstOrigUrl << "->" << strerror(errno);
smb/kio_smb_browse.cpp:        return SMBError{ ERR_INTERNAL, i18n("Unknown error condition in stat: %1", QString::fromLocal8Bit( strerror(errNum))) };
ngraham accepted this revision.Mar 17 2020, 12:22 AM

Let's land this now as-is and make that change universally in a separate patch before we bikeshed it to death. :)

This revision is now accepted and ready to land.Mar 17 2020, 12:22 AM
This revision was automatically updated to reflect the committed changes.