Differential D6830 Diff 17782 src/ioslaves/file/file_unix.cpp

Changeset View

Standalone View

View Options

src/ioslaves/file/file_unix.cpp

Show All 34 Lines
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> 42​#include <KAuth>
43​#include "sharefd.h"
43 44​
44//sendfile has different semantics in different platforms 45​//sendfile has different semantics in different platforms
45#if defined HAVE_SENDFILE && defined Q_OS_LINUX 46​#if defined HAVE_SENDFILE && defined Q_OS_LINUX
46#define USE_SENDFILE 1 47​#define USE_SENDFILE 1
47#endif 48​#endif
48 49​
49#ifdef USE_SENDFILE 50​#ifdef USE_SENDFILE
50#include <sys/sendfile.h> 51​#include <sys/sendfile.h>
▲ Show 20 Lines • Show All 64 Lines • ▼ Show 20 Line(s)114 if (!(_flags & KIO::Overwrite)) {
115 return; 116​ return;
116 } 117​ }
117 118​
118 // If the destination is a symlink and overwrite is TRUE, 119​ // If the destination is a symlink and overwrite is TRUE,
119 // remove the symlink first to prevent the scenario where 120​ // remove the symlink first to prevent the scenario where
120 // the symlink actually points to current source! 121​ // the symlink actually points to current source!
121 if ((_flags & KIO::Overwrite) && ((buff_dest.st_mode & QT_STAT_MASK) == QT_STAT_LNK)) { 122​ if ((_flags & KIO::Overwrite) && ((buff_dest.st_mode & QT_STAT_MASK) == QT_STAT_LNK)) {
122 //qDebug() << "copy(): LINK DESTINATION"; 123​ //qDebug() << "copy(): LINK DESTINATION";
123 QFile::remove(dest); 124​ if (!QFile::remove(dest)) {
125​ if (!execWithElevatedPrivilege(DEL, _dest)) {
126​ error(KIO::ERR_CANNOT_DELETE_ORIGINAL, dest);
dfaureUnsubmitted
Not Done

Missing "return;" no?

dfaure: Missing "return;" no?
127​ return;
128​ }
129​ }
124 } 130​ }
125 } 131​ }
126 132​
127 QFile src_file(src); 133​ QFile src_file(src);
128 if (!src_file.open(QIODevice::ReadOnly)) { 134​ if (!src_file.open(QIODevice::ReadOnly)) {
135​ int src_fd = -1;
136​ FdReceiver fdRecv;
137​ bool _continue = ( fdRecv.startListening(QStringLiteral("org_kde_kio_file_helper_socket"))
138​ && execWithElevatedPrivilege(OPEN, _src, O_RDONLY, S_IRUSR)
139​ && (src_fd = fdRecv.fileDescriptor()) != -1
140​ && src_file.open(src_fd, QIODevice::ReadOnly, QFileDevice::AutoCloseHandle));
141​
142​ if (!_continue) {
129 error(KIO::ERR_CANNOT_OPEN_FOR_READING, src); 143​ error(KIO::ERR_CANNOT_OPEN_FOR_READING, src);
130 return; 144​ return;
131 } 145​ }
146​ }
132 147​
133#if HAVE_FADVISE 148​#if HAVE_FADVISE
134 posix_fadvise(src_file.handle(), 0, 0, POSIX_FADV_SEQUENTIAL); 149​ posix_fadvise(src_file.handle(), 0, 0, POSIX_FADV_SEQUENTIAL);
135#endif 150​#endif
136 151​
137 QFile dest_file(dest); 152​ QFile dest_file(dest);
138 if (!dest_file.open(QIODevice::Truncate | QIODevice::WriteOnly)) { 153​ if (!dest_file.open(QIODevice::Truncate | QIODevice::WriteOnly)) {
139 // qDebug() << "###### COULD NOT WRITE " << dest; 154​ // qDebug() << "###### COULD NOT WRITE " << dest;
155​ int dest_fd = -1;
156​ FdReceiver fdRecv;
157​ bool _continue = ( fdRecv.startListening(QStringLiteral("org_kde_kio_file_helper_socket"))
158​ && execWithElevatedPrivilege(OPEN, _dest, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR)
159​ && (dest_fd = fdRecv.fileDescriptor()) != -1
dfaureUnsubmitted
Not Done

What will fileDescriptor() return if fdRecv isn't valid? -1 I suppose? So this code is going to call open(-1), a bit weird.

Maybe it would be easier if all of the above was moved to a helper function or lambda (with dest_file as input arg).

Basically we want to write

if (auto ret = tryOpen(dest_file, [...])) {
    if (!ret.wasCanceled()) {
    ...

This way you can use early returns in tryOpen when fdRecv isn't valid, when execWithElevatedPrivilege returns anything other than success, or when QFile::open fails. Much better than nested ifs, or missing ifs like above ;)

dfaure: What will fileDescriptor() return if fdRecv isn't valid? -1 I suppose? So this code is going to…
160​ && dest_file.open(dest_fd, QIODevice::WriteOnly | QIODevice::Truncate, QFileDevice::AutoCloseHandle));
161​
162​ if (!_continue) {
140 if (errno == EACCES) { 163​ if (errno == EACCES) {
141 error(KIO::ERR_WRITE_ACCESS_DENIED, dest); 164​ error(KIO::ERR_WRITE_ACCESS_DENIED, dest);
142 } else { 165​ } else {
143 error(KIO::ERR_CANNOT_OPEN_FOR_WRITING, dest); 166​ error(KIO::ERR_CANNOT_OPEN_FOR_WRITING, dest);
144 } 167​ }
145 src_file.close(); 168​ src_file.close();
146 return; 169​ return;
dfaureUnsubmitted
Not Done

One should be enough ;)

dfaure: One should be enough ;)
147 } 170​ }
171​ }
148 172​
149 // nobody shall be allowed to peek into the file during creation 173​ // nobody shall be allowed to peek into the file during creation
150 dest_file.setPermissions(QFileDevice::ReadOwner | QFileDevice::WriteOwner); 174​ if (!QFile::setPermissions(dest, QFileDevice::ReadOwner | QFileDevice::WriteOwner)) {
175​ // File was created by root user with the above permissions. We only need to
dfaureUnsubmitted
Not Done

Was it? How do you know the permissions? Don't they depent on the umask?

Unless I missed something, it would seem to me that a chmod is needed, to ensure restricted permissions. Alternatively, chown + setPermissions again, once the user owns the file.

dfaure: Was it? How do you know the permissions? Don't they depent on the umask? Unless I missed…
176​ // change its owner.
dfaureUnsubmitted
Not Done

missing return;

dfaure: missing return;
177​ if (!execWithElevatedPrivilege(CHOWN, _dest, getuid(), -1)) {
178​ dest_file.close();
179​ execWithElevatedPrivilege(DEL, _dest);
180​ error(KIO::ERR_CANNOT_CHOWN, dest);
181​ return;
182​ }
183​ }
151 184​
152#if HAVE_FADVISE 185​#if HAVE_FADVISE
153 posix_fadvise(dest_file.handle(), 0, 0, POSIX_FADV_SEQUENTIAL); 186​ posix_fadvise(dest_file.handle(), 0, 0, POSIX_FADV_SEQUENTIAL);
154#endif 187​#endif
155 188​
156#if HAVE_POSIX_ACL 189​#if HAVE_POSIX_ACL
157 acl = acl_get_fd(src_file.handle()); 190​ acl = acl_get_fd(src_file.handle());
158 if (acl && !isExtendedACL(acl)) { 191​ if (acl && !isExtendedACL(acl)) {
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Line(s)235#endif
203 error(KIO::ERR_CANNOT_READ, src); 236​ error(KIO::ERR_CANNOT_READ, src);
204 src_file.close(); 237​ src_file.close();
205 dest_file.close(); 238​ dest_file.close();
206#if HAVE_POSIX_ACL 239​#if HAVE_POSIX_ACL
207 if (acl) { 240​ if (acl) {
208 acl_free(acl); 241​ acl_free(acl);
209 } 242​ }
210#endif 243​#endif
211 dest_file.remove(); // don't keep partly copied file 244​ if (!!QFile::remove(dest)) { // don't keep partly copied file
dfaureUnsubmitted
Not Done

Double negation, looks like one too many.

dfaure: Double negation, looks like one too many.
245​ execWithElevatedPrivilege(DEL, _dest);
246​ }
212 return; 247​ return;
213 } 248​ }
214 if (n == 0) { 249​ if (n == 0) {
215 break; // Finished 250​ break; // Finished
216 } 251​ }
217#ifdef USE_SENDFILE 252​#ifdef USE_SENDFILE
218 if (!use_sendfile) { 253​ if (!use_sendfile) {
219#endif 254​#endif
220 if (dest_file.write(buffer, n) != n) { 255​ if (dest_file.write(buffer, n) != n) {
221 if (dest_file.error() == QFileDevice::ResourceError) { // disk full 256​ if (dest_file.error() == QFileDevice::ResourceError) { // disk full
222 error(KIO::ERR_DISK_FULL, dest); 257​ error(KIO::ERR_DISK_FULL, dest);
223 } else { 258​ } else {
224 qCWarning(KIO_FILE) << "Couldn't write[2]. Error:" << dest_file.errorString(); 259​ qCWarning(KIO_FILE) << "Couldn't write[2]. Error:" << dest_file.errorString();
225 error(KIO::ERR_CANNOT_WRITE, dest); 260​ error(KIO::ERR_CANNOT_WRITE, dest);
226 } 261​ }
227#if HAVE_POSIX_ACL 262​#if HAVE_POSIX_ACL
228 if (acl) { 263​ if (acl) {
229 acl_free(acl); 264​ acl_free(acl);
230 } 265​ }
231#endif 266​#endif
232 dest_file.remove(); // don't keep partly copied file 267​ if (!QFile::remove(dest)) { // don't keep partly copied file
268​ execWithElevatedPrivilege(DEL, _dest);
269​ }
233 return; 270​ return;
234 } 271​ }
235 processed_size += n; 272​ processed_size += n;
236#ifdef USE_SENDFILE 273​#ifdef USE_SENDFILE
237 } 274​ }
238#endif 275​#endif
239 processedSize(processed_size); 276​ processedSize(processed_size);
240 } 277​ }
241 278​
242 src_file.close(); 279​ src_file.close();
243 dest_file.close(); 280​ dest_file.close();
244 281​
245 if (dest_file.error() != QFile::NoError) { 282​ if (dest_file.error() != QFile::NoError) {
246 qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString(); 283​ qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString();
247 error(KIO::ERR_CANNOT_WRITE, dest); 284​ error(KIO::ERR_CANNOT_WRITE, dest);
248#if HAVE_POSIX_ACL 285​#if HAVE_POSIX_ACL
249 if (acl) { 286​ if (acl) {
250 acl_free(acl); 287​ acl_free(acl);
251 } 288​ }
252#endif 289​#endif
253 dest_file.remove(); // don't keep partly copied file 290​ if (!!QFile::remove(dest)) { // don't keep partly copied file
dfaureUnsubmitted
Not Done

same

dfaure: same
291​ execWithElevatedPrivilege(DEL, _dest);
292​ }
254 return; 293​ return;
255 } 294​ }
256 295​
257 // set final permissions 296​ // set final permissions
258 // if no special mode given, preserve the mode from the sourcefile 297​ // if no special mode given, preserve the mode from the sourcefile
259 if (_mode == -1) { 298​ if (_mode == -1) {
260 _mode = buff_src.st_mode; 299​ _mode = buff_src.st_mode;
261 } 300​ }
262 301​
263 if ((::chmod(_dest.data(), _mode) != 0) 302​ if ((::chmod(_dest.data(), _mode) != 0)
264#if HAVE_POSIX_ACL 303​#if HAVE_POSIX_ACL
265 || (acl && acl_set_file(_dest.data(), ACL_TYPE_ACCESS, acl) != 0) 304​ || (acl && acl_set_file(_dest.data(), ACL_TYPE_ACCESS, acl) != 0)
266#endif 305​#endif
267 ) { 306​ ) {
307​ if (!execWithElevatedPrivilege(CHMOD, _dest, _mode)) {
268 KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath(dest); 308​ KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath(dest);
269 // Eat the error if the filesystem apparently doesn't support chmod. 309​ // Eat the error if the filesystem apparently doesn't support chmod.
270 if (mp && mp->testFileSystemFlag(KMountPoint::SupportsChmod)) { 310​ if (mp && mp->testFileSystemFlag(KMountPoint::SupportsChmod)) {
271 warning(i18n("Could not change permissions for '%1'", dest)); 311​ warning(i18n("Could not change permissions for '%1'", dest));
272 } 312​ }
273 } 313​ }
314​ }
274#if HAVE_POSIX_ACL 315​#if HAVE_POSIX_ACL
275 if (acl) { 316​ if (acl) {
276 acl_free(acl); 317​ acl_free(acl);
277 } 318​ }
278#endif 319​#endif
279 320​
280 // preserve ownership 321​ // preserve ownership
281 if (::chown(_dest.data(), -1 /*keep user*/, buff_src.st_gid) == 0) { 322​ if (::chown(_dest.data(), -1 /*keep user*/, buff_src.st_gid) == 0) {
282 // as we are the owner of the new file, we can always change the group, but 323​ // as we are the owner of the new file, we can always change the group, but
283 // we might not be allowed to change the owner 324​ // we might not be allowed to change the owner
284 (void)::chown(_dest.data(), buff_src.st_uid, -1 /*keep group*/); 325​ (void)::chown(_dest.data(), buff_src.st_uid, -1 /*keep group*/);
285 } else { 326​ } else {
327​ if (!execWithElevatedPrivilege(CHOWN, _dest, buff_src.st_uid, buff_src.st_gid))
dfaureUnsubmitted
Not Done

Can you test copying a file onto a FAT partition mounted as another uid? The ownership and permissions cannot be applied due to FAT limitations. Does that then trigger a kauth prompt, with this patch? That would be annoying, and wrong since root can't do better anyway.

If I'm right (please test it) then I think this method should remember "I needed root permissions for the main operation" and only in that case use elevated privileges for the small operations at the end like chmod, chown or utime.

Please review whether other methods need the same kind of change.

dfaure: Can you test copying a file onto a FAT partition mounted as another uid? The ownership and…
286 qCWarning(KIO_FILE) << QStringLiteral("Couldn't preserve group for '%1'").arg(dest); 328​ qCWarning(KIO_FILE) << QStringLiteral("Couldn't preserve group for '%1'").arg(dest);
287 } 329​ }
288 330​
289 // copy access and modification time 331​ // copy access and modification time
290 struct utimbuf ut; 332​ struct utimbuf ut;
291 ut.actime = buff_src.st_atime; 333​ ut.actime = buff_src.st_atime;
292 ut.modtime = buff_src.st_mtime; 334​ ut.modtime = buff_src.st_mtime;
293 if (::utime(_dest.data(), &ut) != 0) { 335​ if (::utime(_dest.data(), &ut) != 0) {
336​ // atime and modtime are of type long so not usable with QVariant.
337​ if (!execWithElevatedPrivilege(UTIME, _dest, qint64(ut.actime), qint64(ut.modtime))) {
294 qCWarning(KIO_FILE) << QStringLiteral("Couldn't preserve access and modification time for '%1'").arg(dest); 338​ qCWarning(KIO_FILE) << QStringLiteral("Couldn't preserve access and modification time for '%1'").arg(dest);
295 } 339​ }
dfaureUnsubmitted
Not Done

These cases - without a variable inside the if - could be made more readable with if (exec(...).failed()) { ... } (or whatever the method name was)

dfaure: These cases - without a variable inside the if - could be made more readable with if (exec(...).
340​ }
296 341​
297 processedSize(buff_src.st_size); 342​ processedSize(buff_src.st_size);
298 finished(); 343​ finished();
299} 344​}
300 345​
301static bool isLocalFileSameHost(const QUrl &url) 346​static bool isLocalFileSameHost(const QUrl &url)
302{ 347​{
303 if (!url.isLocalFile()) { 348​ if (!url.isLocalFile()) {
▲ Show 20 Lines • Show All 389 Lines • Show Last 20 Lines