Differential D9966 Diff 26446 src/ioslaves/file/file_unix.cpp

Changeset View

Standalone View

View Options

src/ioslaves/file/file_unix.cpp

Show All 24 Lines
25 25​
26#include "file.h" 26​#include "file.h"
27 27​
28#include <config-kioslave-file.h> 28​#include <config-kioslave-file.h>
29 29​
30#include <QtCore/QFile> 30​#include <QtCore/QFile>
31#include <QtCore/QDir> 31​#include <QtCore/QDir>
32#include <qplatformdefs.h> 32​#include <qplatformdefs.h>
33​#include <QStandardPaths>
33 34​
34#include <QDebug> 35​#include <QDebug>
35#include <kconfiggroup.h> 36​#include <kconfiggroup.h>
36#include <klocalizedstring.h> 37​#include <klocalizedstring.h>
37#include <kmountpoint.h> 38​#include <kmountpoint.h>
38 39​
39#include <errno.h> 40​#include <errno.h>
40#include <utime.h> 41​#include <utime.h>
41 42​
42#include <KAuth> 43​#include <KAuth>
44​#include <KRandom>
43 45​
44#include "fdreceiver.h" 46​#include "fdreceiver.h"
45 47​
46//sendfile has different semantics in different platforms 48​//sendfile has different semantics in different platforms
47#if defined HAVE_SENDFILE && defined Q_OS_LINUX 49​#if defined HAVE_SENDFILE && defined Q_OS_LINUX
48#define USE_SENDFILE 1 50​#define USE_SENDFILE 1
49#endif 51​#endif
50 52​
Show All 13 Lines64 if (src.st_ino == dest.st_ino &&
64 return true; 66​ return true;
65 } 67​ }
66 68​
67 return false; 69​ return false;
68} 70​}
69 71​
70static const QString socketPath() 72​static const QString socketPath()
71{ 73​{
72 return QStringLiteral("org_kde_kio_file_helper_%1").arg(getpid()); 74​ const QString runtimeDir = QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation);
75​ return QStringLiteral("%1/filehelper%2%3").arg(runtimeDir).arg(KRandom::randomString(6)).arg(getpid());
73} 76​}
74 77​
75bool FileProtocol::privilegeOperationUnitTestMode() 78​bool FileProtocol::privilegeOperationUnitTestMode()
76{ 79​{
77 return (metaData(QStringLiteral("UnitTesting")) == QLatin1String("true")) 80​ return (metaData(QStringLiteral("UnitTesting")) == QLatin1String("true"))
78 && (requestPrivilegeOperation() == KIO::OperationAllowed); 81​ && (requestPrivilegeOperation() == KIO::OperationAllowed);
79} 82​}
80 83​
81PrivilegeOperationReturnValue FileProtocol::tryOpen(QFile &f, const QByteArray &path, int flags, int mode, int errcode) 84​PrivilegeOperationReturnValue FileProtocol::tryOpen(QFile &f, const QByteArray &path, int flags, int mode, int errcode)
82{ 85​{
83 const QString sockPath = socketPath(); 86​ const QString sockPath = socketPath();
84 FdReceiver fdRecv(sockPath); 87​ FdReceiver fdRecv(sockPath);
85 if (!fdRecv.isListening()) { 88​ if (!fdRecv.isListening()) {
86 return PrivilegeOperationReturnValue::failure(errcode); 89​ return PrivilegeOperationReturnValue::failure(errcode);
87 } 90​ }
88 91​
89 QIODevice::OpenMode openMode; 92​ QIODevice::OpenMode openMode;
fvogtUnsubmitted
Not Done

That looks like a hack. If errno is that important, save and keep it in a variable. Every call into a library can change errno, it's extremely volatile.

fvogt: That looks like a hack. If errno is that important, save and keep it in a variable. Every call…
chinmoyrAuthorUnsubmitted
Not Done

It is the only case for which this hack seems necessary. For all other cases a library call (to perform a file operation) is immediately followed by a call to helper. IMO the chances of errno changing to something unrelated in between these two calls are very slim (is it even possible?)
Although errno is important, saving it for every call will result in unnecessary code. Can't we make an exception for this case?

chinmoyr: It is the only case for which this hack seems necessary. For all other cases a library call (to…
fvogtUnsubmitted
Not Done

I don't see how this could ever work. Even the line immediately below errno = err can change errno.
You must not assume that errno does not change if you call a function. Save it immediately after the function which errno you are interested in returns.
The famous "Could not perform operation: Success" - kind of error messages happens exactly because of bugs like these.

fvogt: I don't see how this could ever work. Even the line immediately below `errno = err` can change…
90 if (flags & O_RDONLY) { 93​ if (flags & O_RDONLY) {
91 openMode |= QIODevice::ReadOnly; 94​ openMode |= QIODevice::ReadOnly;
92 } 95​ }
93 if (flags & O_WRONLY || flags & O_CREAT) { 96​ if (flags & O_WRONLY || flags & O_CREAT) {
94 openMode |= QIODevice::WriteOnly; 97​ openMode |= QIODevice::WriteOnly;
95 } 98​ }
96 if (flags & O_RDWR) { 99​ if (flags & O_RDWR) {
97 openMode |= QIODevice::ReadWrite; 100​ openMode |= QIODevice::ReadWrite;
98 } 101​ }
99 if (flags & O_TRUNC) { 102​ if (flags & O_TRUNC) {
100 openMode |= QIODevice::Truncate; 103​ openMode |= QIODevice::Truncate;
101 } 104​ }
102 if (flags & O_APPEND) { 105​ if (flags & O_APPEND) {
103 openMode |= QIODevice::Append; 106​ openMode |= QIODevice::Append;
104 } 107​ }
105 108​
106 if (auto err = execWithElevatedPrivilege(OPEN, {path, flags, mode, sockPath}, errcode)) { 109​ if (auto err = execWithElevatedPrivilege(OPEN, {path, flags, mode, sockPath}, errcode)) {
107 return err; 110​ return err;
chinmoyrAuthorUnsubmitted
Not Done

@fvogt I think the errno assignment should be fine here?
Except this one case, the function call whose errno value we are interested in is immediately followed by execWithElevatedPrivilege (which checks errno as the first thing). So I was saying earlier that saving errno there isn't necessary because there is no (other) library call between the failed function call and execWithElevatedPrivilege.

chinmoyr: @fvogt I think the errno assignment should be fine here? Except this one case, the function…
fvogtUnsubmitted
Done

This looks very much like code smell. Currently you treat errno as a hidden function parameter.
IMO you should make it explicit.

fvogt: This looks very much like code smell. Currently you treat `errno` as a hidden function…
108 } else { 111​ } else {
109 int fd = fdRecv.fileDescriptor(); 112​ int fd = fdRecv.fileDescriptor();
110 if (fd < 3 || !f.open(fd, openMode, QFileDevice::AutoCloseHandle)) { 113​ if (fd < 3 || !f.open(fd, openMode, QFileDevice::AutoCloseHandle)) {
111 return PrivilegeOperationReturnValue::failure(errcode); 114​ return PrivilegeOperationReturnValue::failure(errcode);
112 } 115​ }
113 } 116​ }
114 return PrivilegeOperationReturnValue::success(); 117​ return PrivilegeOperationReturnValue::success();
115} 118​}
▲ Show 20 Lines • Show All 715 Lines • Show Last 20 Lines