Changeset View
Standalone View
src/ioslaves/file/file_unix.cpp
Show All 33 Lines | |||||
34 | #include <QDebug> | 34 | #include <QDebug> | ||
35 | #include <kconfiggroup.h> | 35 | #include <kconfiggroup.h> | ||
36 | #include <klocalizedstring.h> | 36 | #include <klocalizedstring.h> | ||
37 | #include <kmountpoint.h> | 37 | #include <kmountpoint.h> | ||
38 | 38 | | |||
39 | #include <errno.h> | 39 | #include <errno.h> | ||
40 | #include <utime.h> | 40 | #include <utime.h> | ||
41 | 41 | | |||
42 | #include <KAuth> | ||||
43 | | ||||
42 | //sendfile has different semantics in different platforms | 44 | //sendfile has different semantics in different platforms | ||
43 | #if defined HAVE_SENDFILE && defined Q_OS_LINUX | 45 | #if defined HAVE_SENDFILE && defined Q_OS_LINUX | ||
44 | #define USE_SENDFILE 1 | 46 | #define USE_SENDFILE 1 | ||
45 | #endif | 47 | #endif | ||
46 | 48 | | |||
47 | #ifdef USE_SENDFILE | 49 | #ifdef USE_SENDFILE | ||
48 | #include <sys/sendfile.h> | 50 | #include <sys/sendfile.h> | ||
49 | #endif | 51 | #endif | ||
50 | 52 | | |||
51 | using namespace KIO; | 53 | using namespace KIO; | ||
52 | 54 | | |||
53 | #define MAX_IPC_SIZE (1024*32) | 55 | #define MAX_IPC_SIZE (1024*32) | ||
54 | 56 | | |||
55 | static bool | 57 | static bool | ||
56 | same_inode(const QT_STATBUF &src, const QT_STATBUF &dest) | 58 | same_inode(const QT_STATBUF &src, const QT_STATBUF &dest) | ||
57 | { | 59 | { | ||
58 | if (src.st_ino == dest.st_ino && | 60 | if (src.st_ino == dest.st_ino && | ||
59 | src.st_dev == dest.st_dev) { | 61 | src.st_dev == dest.st_dev) { | ||
60 | return true; | 62 | return true; | ||
61 | } | 63 | } | ||
62 | 64 | | |||
63 | return false; | 65 | return false; | ||
64 | } | 66 | } | ||
65 | 67 | | |||
68 | const QString socketPath() | ||||
69 | { | ||||
70 | return QStringLiteral("org_kde_kio_file_helper_%1").arg(getpid()); | ||||
71 | } | ||||
72 | | ||||
66 | void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, | 73 | void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, | ||
67 | int _mode, JobFlags _flags) | 74 | int _mode, JobFlags _flags) | ||
68 | { | 75 | { | ||
69 | // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode; | 76 | // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode; | ||
70 | 77 | | |||
71 | const QString src = srcUrl.toLocalFile(); | 78 | const QString src = srcUrl.toLocalFile(); | ||
72 | const QString dest = destUrl.toLocalFile(); | 79 | const QString dest = destUrl.toLocalFile(); | ||
73 | QByteArray _src(QFile::encodeName(src)); | 80 | QByteArray _src(QFile::encodeName(src)); | ||
▲ Show 20 Lines • Show All 571 Lines • ▼ Show 20 Line(s) | 651 | for (; it1 != mOutgoingMetaData.end(); it1++) { | |||
645 | // qDebug() << it1.key() << " = " << it1.data(); | 652 | // qDebug() << it1.key() << " = " << it1.data(); | ||
646 | } | 653 | } | ||
647 | ///////// | 654 | ///////// | ||
648 | #endif | 655 | #endif | ||
649 | statEntry(entry); | 656 | statEntry(entry); | ||
650 | 657 | | |||
651 | finished(); | 658 | finished(); | ||
652 | } | 659 | } | ||
660 | | ||||
661 | PrivilegeOperationReturnValue FileProtocol::execWithElevatedPrivilege(ActionType action, const QVariant &arg1, | ||||
662 | const QVariant &arg2, const QVariant &arg3) | ||||
663 | { | ||||
664 | if (!(errno == EACCES || errno == EPERM)) { | ||||
dfaure: with EPERM we should try as root too, no?
(e.g. for unlink, rename, chown...) | |||||
665 | return PrivilegeOperationReturnValue::failure(); | ||||
666 | } | ||||
667 | | ||||
668 | KIO::PrivilegeOperationStatus opStatus = requestPrivilegeOperation(); | ||||
669 | if (opStatus != KIO::OperationAllowed) { | ||||
670 | if (opStatus == KIO::OperationCanceled) { | ||||
Wait, why the two step roundtrip here? Which even removes the need for any enum, it's really just one request and one reply which is true or false. dfaure: Wait, why the two step roundtrip here?
Instead of "can you become root?" / "yes I can" / "ok… | |||||
671 | error(KIO::ERR_USER_CANCELED, QString()); | ||||
Who's going to read that value? The caller? Abusing errno to ship a return value via global state reads horrible to me.... we're not a libc function.... dfaure: Who's going to read that value? The caller? Abusing errno to ship a return value via global… | |||||
No chance to return an enum instead of a bool here, to avoid modifying state? The problem with mUserCanceled is that it's easy to get it wrong. Like in this patch, which never resets it to false for the next operation after one canceled operation. It's just cleaner to return an enum instead of modifying a member variable temporarily, although the question is, in both cases, what the caller code will look like... My fear with a member var is that too many callers will forget to check it. Before: if (!dir.rmdir(itemPath)) { error(KIO::ERR_CANNOT_DELETE, itemPath); return false; } After: if (!dir.rmdir(itemPath)) { if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { // for this to work, success must be 0 (or the returned type can be a class with operator bool()...) if (ret == KIO::OperationCanceled) { // or a different enum error(KIO::ERR_USER_CANCELED, itemPath); } else { error(KIO::ERR_CANNOT_DELETE, itemPath); } return false; } } Alternatively, since we know cancelling will always need to error(KIO::ERR_USER_CANCELED) (and the QString argument doesn't matter), we could call error from within execWithElevatedPrivilege. But we still need the enum ret val: if (!dir.rmdir(itemPath)) { if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { // see above if (ret != KIO::OperationCanceled) { // or a different enum error(KIO::ERR_CANNOT_DELETE, itemPath); } return false; } } I actually like the idea of returning a small class. Then it could even have a wasCanceled() method... we don't need an enum, we need a value that can convert to bool (for the if) and additionally let us know if cancelation happened. But all this in a temporary value, not in a member var of slavebase. To flesh it out in more details: class ExecWithElevatedPrivilegeReturnValue { public: static ExecWithElevatedPrivilegeReturnValue success() { return ExecWithElevatedPrivilegeReturnValue{true, false}; } static ExecWithElevatedPrivilegeReturnValue failure() { return ExecWithElevatedPrivilegeReturnValue{true, false}; } static ExecWithElevatedPrivilegeReturnValue canceled() { return ExecWithElevatedPrivilegeReturnValue{true, true}; } operator bool() const { return m_success; } bool wasCanceled() const { return m_canceled; } private: ExecWithElevatedPrivilegeReturnValue(bool success, bool canceled) : m_success(success), m_canceled(canceled) {} const bool m_success; const bool m_canceled; }; (no setters, all of the slave code shouldn't be able to change this object, it's created immediately with the right values) With this, the calling code can be simply if (!dir.rmdir(itemPath)) { if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { if (!ret.wasCanceled()) { error(KIO::ERR_CANNOT_DELETE, itemPath); } return false; } } and the code in this review (execWithElevatedPrivilege) can do things like return ExecWithElevatedPrivilegeReturnValue::canceled(); etc. How does that sound? dfaure: No chance to return an enum instead of a bool here, to avoid modifying state?
The problem with… | |||||
672 | return PrivilegeOperationReturnValue::canceled(); | ||||
673 | } | ||||
674 | return PrivilegeOperationReturnValue::failure(); | ||||
675 | } | ||||
676 | | ||||
677 | KAuth::Action execAction(QStringLiteral("org.kde.kio.file.exec")); | ||||
678 | execAction.setHelperId(QStringLiteral("org.kde.kio.file")); | ||||
679 | | ||||
680 | // If we are unit testing let's pretend to execute the action. | ||||
681 | if (metaData(QStringLiteral("UnitTesting")) == QLatin1String("true")) { | ||||
682 | const QString metaData = execAction.name() + ',' | ||||
dfaure: single-quotes for the commas (','), like in my earlier recommendation. | |||||
683 | + (execAction.isValid() ? "true" : "false") + ',' | ||||
684 | + execAction.helperId() + ',' | ||||
685 | + (execAction.status() == KAuth::Action::AuthRequiredStatus ? "true" : "false"); | ||||
686 | setMetaData(QStringLiteral("TestData"), metaData); | ||||
687 | return PrivilegeOperationReturnValue::success(); | ||||
688 | } | ||||
689 | | ||||
690 | if (action == CHMOD || action == CHOWN || action == UTIME) { | ||||
691 | KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath(arg1.toString()); | ||||
use C++11 initializer to avoid reallocations, i.e. QStringList testData{execAction.name(), ..., ..., ...}; Ah but then you just use a join(","), so the most efficient thing to do here would be to drop the QStringList and do const QString metaData = execAction.name() + ',' + QString::number... + ',' + etc. dfaure: use C++11 initializer to avoid reallocations, i.e. `QStringList testData{execAction.name(), ... | |||||
692 | // Test for chmod and utime will return the same result as test for chown. | ||||
693 | if (mp && !mp->testFileSystemFlag(KMountPoint::SupportsChown)) { | ||||
694 | return PrivilegeOperationReturnValue::failure(); | ||||
695 | } | ||||
696 | } | ||||
697 | | ||||
698 | QByteArray helperArgs; | ||||
699 | QDataStream out(&helperArgs, QIODevice::WriteOnly); | ||||
700 | out << action << arg1 << arg2 << arg3; | ||||
701 | | ||||
702 | if (action == OPEN || action == OPENDIR) { | ||||
703 | out << socketPath(); | ||||
704 | } | ||||
705 | | ||||
706 | QVariantMap argv; | ||||
707 | argv.insert(QStringLiteral("arguments"), helperArgs); | ||||
708 | execAction.setArguments(argv); | ||||
709 | | ||||
710 | auto reply = execAction.execute(); | ||||
711 | if (reply->exec()) { | ||||
712 | return PrivilegeOperationReturnValue::success(); | ||||
713 | } | ||||
714 | | ||||
715 | return PrivilegeOperationReturnValue::failure(); | ||||
716 | } |
with EPERM we should try as root too, no?
(e.g. for unlink, rename, chown...)