Differential D17816 Diff 83207 src/ioslaves/file/file_unix.cpp

Changeset View

Standalone View

View Options

src/ioslaves/file/file_unix.cpp

Show All 32 Lines
33#include <QStandardPaths> 33​#include <QStandardPaths>
34 34​
35#include <QDebug> 35​#include <QDebug>
36#include <kconfiggroup.h> 36​#include <kconfiggroup.h>
37#include <klocalizedstring.h> 37​#include <klocalizedstring.h>
38#include <kmountpoint.h> 38​#include <kmountpoint.h>
39 39​
40#include <errno.h> 40​#include <errno.h>
41#if HAVE_SYS_XATTR_H ​
42#include <sys/xattr.h> ​
43#endif ​
44#include <utime.h> 41​#include <utime.h>
pinoUnsubmitted
Done

HAVE_SYS_XATTR_H from config-kioslave-file.h can be used here

pino: `HAVE_SYS_XATTR_H` from `config-kioslave-file.h` can be used here
pinoUnsubmitted
Done

better check for sys/extattr.h instead

pino: better check for `sys/extattr.h` instead
45 42​
46#include <KAuth> 43​#include <KAuth>
47#include <KRandom> 44​#include <KRandom>
48 45​
49#include "fdreceiver.h" 46​#include "fdreceiver.h"
50 47​
51//sendfile has different semantics in different platforms 48​//sendfile has different semantics in different platforms
52#if HAVE_SENDFILE && defined Q_OS_LINUX 49​#if HAVE_SENDFILE && defined Q_OS_LINUX
53#define USE_SENDFILE 1 50​#define USE_SENDFILE 1
54#endif 51​#endif
55 52​
56#ifdef USE_SENDFILE 53​#ifdef USE_SENDFILE
57#include <sys/sendfile.h> 54​#include <sys/sendfile.h>
58#endif 55​#endif
59 56​
57​#if HAVE_SYS_XATTR_H
58​#include <sys/xattr.h>
59​//BSD uses a different include
60​#elif HAVE_SYS_EXTATTR_H
61​#include <sys/extattr.h>
62​#endif
63​
60using namespace KIO; 64​using namespace KIO;
61 65​
62#define MAX_IPC_SIZE (1024*32) 66​#define MAX_IPC_SIZE (1024*32)
63 67​
64static bool 68​static bool
65same_inode(const QT_STATBUF &src, const QT_STATBUF &dest) 69​same_inode(const QT_STATBUF &src, const QT_STATBUF &dest)
66{ 70​{
67 if (src.st_ino == dest.st_ino && 71​ if (src.st_ino == dest.st_ino &&
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Line(s)128{
125 KAuth::Action execAction(QStringLiteral("org.kde.kio.file.exec")); 129​ KAuth::Action execAction(QStringLiteral("org.kde.kio.file.exec"));
126 execAction.setHelperId(QStringLiteral("org.kde.kio.file")); 130​ execAction.setHelperId(QStringLiteral("org.kde.kio.file"));
127 if (execAction.status() == KAuth::Action::AuthorizedStatus) { 131​ if (execAction.status() == KAuth::Action::AuthorizedStatus) {
128 return execWithElevatedPrivilege(action, args, errcode); 132​ return execWithElevatedPrivilege(action, args, errcode);
129 } 133​ }
130 return PrivilegeOperationReturnValue::failure(errcode); 134​ return PrivilegeOperationReturnValue::failure(errcode);
131} 135​}
132 136​
137​
138​bool FileProtocol::copyXattrs(const int src_fd, const int dest_fd)
dfaureUnsubmitted
Done

This whole method should be in #if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H.

Otherwise, on systems with neither defined, a lot of compiler warnings will happen, like "keyLen isn't initialized" on line 591.

You can leave the method in the .h without #if (those defines aren't known there).
Here you can either #if the method itself (declared and not defined and not used, is OK, although some might say, a bit unclean)
or #if the whole method *contents*, but then in the #else you need Q_UNUSED(src_fd); Q_UNUSED(dest_fd); to avoid compiler warnings about unused parameters.

dfaure: This whole method should be in `#if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H`. Otherwise, on…
139​{
140​ // Get the list of keys
dfaureUnsubmitted
Done

typo: soure => source

dfaure: typo: soure => source
141​ ssize_t listlen = 0;
brunsUnsubmitted
Done

The idea is almost right, the implementation is wrong.

1. set listlen to 0

2. resize keylist to listlen
3. execute flistxattr with size = keylist.size();
4a. if (3.) returns listlen > 0 and keylist.size() == 0, go to (2.)
4b. if (3.) returns listlen > 0 and keylist.size() > 0 -> break
4c. if (3.) returns listlen == -1 and errno == ERANGE, set listlen to 0 and go to (2.)
4d. if (3.) returns listlen == -1 and errno == ENOTSUP -> return
4e. if (3.) returns listlen == 0 -> return

5. resize keylist to listlen, the list may shrink between flistxattr calls.
bruns: The idea is almost right, the implementation is wrong. ``` 1. set listlen to 0 2. resize…
142​ QByteArray keylist;
brunsUnsubmitted
Done

just QByteArray keylist;

bruns: just `QByteArray keylist;`
143​ while (true) {
brunsUnsubmitted
Done

while (true) instead of do {} while(true)

bruns: `while (true)` instead of `do {} while(true)`
144​ keylist.resize(listlen);
145​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
brunsUnsubmitted
Done

extattr_list_fd, here and everywhere else

bruns: extattr_list_**fd**, here and everywhere else
146​ listlen = flistxattr(src_fd, keylist.data(), listlen);
147​#elif defined(Q_OS_MAC)
148​ listlen = flistxattr(src_fd, keylist.data(), listlen, 0);
brunsUnsubmitted
Done

You never check if the source FS supports xattrs.

bruns: You never check if the source FS supports xattrs.
149​#elif HAVE_SYS_EXTATTR_H
brunsUnsubmitted
Done

Now try this when the source is e.g. FAT formatted ...

Make this dependent on errno.

bruns: Now try this when the source is e.g. FAT formatted ... Make this dependent on errno.
150​ listlen = extattr_list_fd(src_fd, EXTATTR_NAMESPACE_USER, keylist.data(), listlen);
151​#endif
152​ if (listlen > 0 && keylist.size() == 0) {
153​ continue;
dfaureUnsubmitted
Done

No need for spaces (after "file" and before "don't"), q[C]Debug inserts spaces automatically

dfaure: No need for spaces (after "file" and before "don't"), q[C]Debug inserts spaces automatically
brunsUnsubmitted
Done

as listlen is -1 here, you always resize to 511 ...

bruns: as listlen is -1 here, you always resize to 511 ...
154​ }
155​ if (listlen > 0 && keylist.size() > 0) {
brunsUnsubmitted
Done

This is still definitely wrong ...

bruns: This is still definitely wrong ...
156​ break;
157​ }
158​ if (listlen == -1 && errno == ERANGE) {
159​ listlen = 0;
160​ continue;
161​ }
162​ if (listlen == -1) {
163​ if (errno == ENOTSUP) {
dfaureUnsubmitted
Done

Can this *ever* return an empty list, because keylist was empty?

(Then last() will assert on the next line)

dfaure: Can this *ever* return an empty list, because keylist was empty? (Then last() will assert on…
brunsUnsubmitted
Done

Why a temporary list at all?

off_t offset = 0; size_t keyLen;
while (offset < keylist.size()) {
#if BSD
   keyLen = static_cast<unsigned char>(keylist[offset]);
   offset++;
#elif LINUX
    keyLen = strlen(keylist[offset]
#endif
    key = keylist.mid(offset, keyLen);
    /* copy */
...
#if BSD
    offset += keyLen;
#elif LINUX
    offset += keyLen + 1;
#endif
}
bruns: Why a temporary list at all? ``` off_t offset = 0; size_t keyLen; while (offset < keylist.size…
cochiseUnsubmitted
Done

As the system functions need individual keys to get values and a individual key value pair to set, the list is to make more easy to iterate over keys. We can iterate over the buffer, reading until '\0', but this is a little more bug prone.

cochise: As the system functions need individual keys to get values and a individual key value pair to…
cochiseUnsubmitted
Done

Not because if the file don't have a xattr the function returns on 153. The returned keylist should have at least one key.
Removing last, if empy is done because in my tests the keylist includes a empty attribute that is preserved, and appended after each copy.

if (listlen == 0) {
        qCDebug(KIO_FILE) << "file " << src_fd << " don't have any xattr";
        return false;
}
cochise: Not because if the file don't have a xattr the function returns on 153. The returned keylist…
brunsUnsubmitted
Done

This is wrong for the BSD implementation:

extattr_list_file() returns a list of attributes present in the requested
namespace. Each list entry consists of a single byte containing the
length of the attribute name, followed by the attribute name. The attri-
bute name is not terminated by ASCII 0 (nul).

bruns: This is wrong for the BSD implementation: > extattr_list_file() returns a list of attributes…
cochiseUnsubmitted
Done

Well... This will need some platform specif code to read the buffer and place in the m_keyList, ad the .split() will not work. Using the list is mandatory now, as we have different format buffers.
Committing other fixes, ad they are really small and postponed this one.

cochise: Well... This will need some platform specif code to read the buffer and place in the m_keyList…
brunsUnsubmitted
Done

man 2 flistxattr:

In addition, the errors documented in stat(2) can also occur.

So any error but ERANGE will exit the loop, and you will have bogus keylist contents.

Do return false on any error but ERANGE in the loop.

After the loop (i.e. when listlen is guaranteed to be > 0), resize keylist to listlen, the list may shrink.

bruns: man 2 flistxattr: > In addition, the errors documented in stat(2) can also occur. So any error…
164​ qCDebug(KIO_FILE) << "source filesystem does not support xattrs";
brunsUnsubmitted
Done

"does not"

bruns: "does not"
165​ }
166​ return false;
dfaureUnsubmitted
Done

typo: srting => string

dfaure: typo: srting => string
167​ }
ustaUnsubmitted
Done

small typo in comment : itens => items

usta: small typo in comment : itens => items
168​ if (listlen == 0) {
169​ qCDebug(KIO_FILE) << "file" << src_fd << "don't have any xattr";
dfaureUnsubmitted
Done

To me an offset is an integer that represents a relative index.

This is more like a source pointer, or keyData or something like that.

dfaure: To me an offset is an integer that represents a relative index. This is more like a source…
brunsUnsubmitted
Done

src_fd is quite meaningless as debug output

bruns: `src_fd` is quite meaningless as debug output
170​ return false;
171​ }
dfaureUnsubmitted
Done

What's the difference with QByteArray key; ?

dfaure: What's the difference with `QByteArray key;` ?
172​ }
brunsUnsubmitted
Done

Should be true

bruns: Should be `true`
173​
brunsUnsubmitted
Done

Thats bad. Thats really bad.

squeeze may reallocate. This invalidates keyPtr.

bruns: Thats bad. Thats really bad. squeeze may reallocate. This invalidates keyPtr.
174​ keylist.resize(listlen);
175​
brunsUnsubmitted
Done

why .squeeze() ?

bruns: why `.squeeze()` ?
176​ // Linux and MacOS return a list of null terminated strings, each string = [data,'\0']
177​ // BSDs return a list of items, each item consisting of the size byte
brunsUnsubmitted
Done

"return a list of null terminated strings"

bruns: "return a list of null terminated strings"
178​ // prepended to the key = [size, data]
brunsUnsubmitted
Done

"BSDs return a list of items, ..." or "BSD returns a list of items, ..."
"..., each item consisting of the size byte prepended to the key."

bruns: "BSDs return a list of items, ..." or "BSD returns a list of items, ..." "..., each item…
179​ QByteArray::const_iterator keyPtr = keylist.cbegin();
brunsUnsubmitted
Done

Allocate outside the loop

bruns: Allocate outside the loop
cochiseUnsubmitted
Done

The value change on every pass and we only get the size inside the loop. Allocating outside the loop is more performant than resizing for each pass?

cochise: The value change on every pass and we only get the size inside the loop. Allocating outside the…
dfaureUnsubmitted
Done

Ah, I see, you still need to call strlen() yourself, for the code at the end of the iteration....

Well, then you might as well pass the value to the constructor so it doesn't need to do the same.

const QByteArray key(keyPtr, keyLen);

after the #endif, since it would now be the exact same line of code for both cases.

dfaure: Ah, I see, you still need to call `strlen()` yourself, for the code at the end of the iteration.
cochiseUnsubmitted
Done

This look really better.

cochise: This look really better.
180​ size_t keyLen;
brunsUnsubmitted
Done

*keyPtr needs cast to unsigned char.

bruns: `*keyPtr` needs cast to unsigned char.
181​ QByteArray value;
182​
183​ // For each key
dfaureUnsubmitted
Done

What's the purpose of these two lines modifying key, given that the next line assigns to key anyway?

dfaure: What's the purpose of these two lines modifying `key`, given that the next line assigns to…
cochiseUnsubmitted
Done

Removed clear, as is redundant.

cochise: Removed clear, as is redundant.
184​ while (keyPtr != keylist.cend()) {
brunsUnsubmitted
Done

valuelen

bruns: valuelen
dfaureUnsubmitted
Done

You're doing two copies here. From offset to key.data(), and then from key.data() -- the return value of qstrcpy -- into the QByteArray key (which calls the QByteArray(const char*) constructor).

This can be simplified.
Option 1: just remove the assignment, just do the qstrcpy. But it still smells like C to me. And there's a security bug if keyLen is ever too small.
Option 2: QByteArray key(offset); So simple. No need for strlen before, no need for qstrcpy, it all happens internally in that constructor.

dfaure: You're doing two copies here. From `offset` to `key.data()`, and then from `key.data()` -- the…
cochiseUnsubmitted
Done

So many time looking for a way to get a copy until a null, and it's in one of the constructors.
This looks a lot better.

cochise: So many time looking for a way to get a copy until a null, and it's in one of the constructors.
185​ // Get size of the key
brunsUnsubmitted
Done

"key size" or "size of the key"

bruns: "key size" or "size of the key"
186​#if HAVE_SYS_XATTR_H
brunsUnsubmitted
Done

valuelen

bruns: valuelen
dfaureUnsubmitted
Done

Same problem here. Similar solution: QByteArray key(offset, keyLen);

This removes the need for key.resize() before. Which also removes the need for the two parallel series of ifdefs, you can just merge them.

dfaure: Same problem here. Similar solution: QByteArray key(offset, keyLen); This removes the need for…
brunsUnsubmitted
Not Done

QByteArray value should be created outside the keyPtr loop.

This reduces the calls to malloc for the QByteArray data storage.

Then for each attribute, do a value.resize(value.capacity).

bruns: `QByteArray value` should be created outside the keyPtr loop. This reduces the calls to malloc…
187​ keyLen = strlen(keyPtr);
188​#elif HAVE_SYS_EXTATTR_H
189​ keyLen = static_cast<unsigned char>(*keyPtr);
190​ keyPtr++;
191​#endif
192​ QByteArray key(keyPtr, keyLen);
193​
194​ // Get the value for key
195​ ssize_t valuelen = 0;
196​ do {
brunsUnsubmitted
Done

-1 + 512 = 511 ...

Should be value.resize(value.size() + 512) or something similar.

bruns: -1 + 512 = 511 ... Should be `value.resize(value.size() + 512)` or something similar.
197​ value.resize(valuelen);
198​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
brunsUnsubmitted
Done

you can return here, no value in trying again and again ...

bruns: you can return here, no value in trying again and again ...
brunsUnsubmitted
Done

Again, valuelen may be -1 here ...

bruns: Again, valuelen may be -1 here ...
199​ valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen);
200​#elif defined(Q_OS_MAC)
brunsUnsubmitted
Not Done

also wrong ...

bruns: also wrong ...
201​ valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen, 0, 0);
202​#elif HAVE_SYS_EXTATTR_H
203​ valuelen = extattr_get_fd(src_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.data(), valuelen);
204​#endif
205​ if (valuelen > 0 && value.size() == 0) {
206​ continue;
brunsUnsubmitted
Done

WRONG WRONG WRONG !!!

bruns: WRONG WRONG WRONG !!!
207​ }
208​ if (valuelen > 0 && value.size() > 0) {
209​ break;
brunsUnsubmitted
Done

No extra spaces for qDebug, inserts spaces itself.

bruns: No extra spaces for qDebug, inserts spaces itself.
210​ }
211​ if (valuelen == -1 && errno == ERANGE) {
212​ valuelen = 0;
213​ continue;
214​ }
brunsUnsubmitted
Done

Infinite loop on valuelen == 0

bruns: Infinite loop on valuelen == 0
arrowdAuthorUnsubmitted
Done

Why? ERANGE means we need to come up with new value for valuelen, so we set it to zero and start over. On the new iteration it gets passed into fgetxattr/extattr_get_fd to find out sufficient value.

arrowd: Why? `ERANGE` means we need to come up with new value for `valuelen`, so we set it to zero and…
brunsUnsubmitted
Done

"0" is a regular return value. Try your code with the following file:

touch t
setfattr -n user.foo -v "" t

and preferably add a test case ...

bruns: "0" is a regular return value. Try your code with the following file: ``` touch t setfattr -n…
215​ } while (true);
216​
217​ // Write key:value pair on destination
218​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
219​ ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.constData(), valuelen, 0);
220​#elif defined(Q_OS_MAC)
221​ ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.constData(), valuelen, 0, 0);
222​#elif HAVE_SYS_EXTATTR_H
223​ ssize_t destlen = extattr_set_fd(dest_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.constData(), valuelen);
224​#endif
225​ if (destlen == -1 && errno == ENOTSUP) {
226​ qCDebug(KIO_FILE) << "Destination filesystem does not support xattrs";
brunsUnsubmitted
Done

"does not"

bruns: "does not"
227​ return false;
228​ }
229​
230​#if HAVE_SYS_XATTR_H
231​ keyPtr += keyLen + 1;
232​#elif HAVE_SYS_EXTATTR_H
brunsUnsubmitted
Done

what about #elif defined(Q_OS_MAC)?

bruns: what about `#elif defined(Q_OS_MAC)`?
dfaureUnsubmitted
Done

@bruns HAVE_SYS_XATTR_H covers mac and non-mac here.

dfaure: @bruns HAVE_SYS_XATTR_H covers mac and non-mac here.
233​ keyPtr += keyLen;
234​#endif
235​ }
236​ return true;
237​}
238​
133void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, 239​void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl,
134 int _mode, JobFlags _flags) 240​ int _mode, JobFlags _flags)
135{ 241​{
136 if (privilegeOperationUnitTestMode()) { 242​ if (privilegeOperationUnitTestMode()) {
137 finished(); 243​ finished();
138 return; 244​ return;
139 } 245​ }
140 246​
▲ Show 20 Lines • Show All 183 Lines • ▼ Show 20 Line(s)425#endif
324 } 430​ }
325 processed_size += n; 431​ processed_size += n;
326#ifdef USE_SENDFILE 432​#ifdef USE_SENDFILE
327 } 433​ }
328#endif 434​#endif
329 processedSize(processed_size); 435​ processedSize(processed_size);
330 } 436​ }
331 437​
438​ // Copy Extended attributes
439​#if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H
440​ if (!copyXattrs(src_file.handle(), dest_file.handle())) {
441​ qCDebug(KIO_FILE) << "cant copy Extended attributes";
442​ }
443​#endif
444​
332 src_file.close(); 445​ src_file.close();
333 dest_file.close(); 446​ dest_file.close();
334 447​
335 if (dest_file.error() != QFile::NoError) { 448​ if (dest_file.error() != QFile::NoError) {
336 qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString(); 449​ qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString();
337 error(KIO::ERR_CANNOT_WRITE, dest); 450​ error(KIO::ERR_CANNOT_WRITE, dest);
338#if HAVE_POSIX_ACL 451​#if HAVE_POSIX_ACL
339 if (acl) { 452​ if (acl) {
▲ Show 20 Lines • Show All 77 Lines • ▼ Show 20 Line(s)
417} 530​}
418 531​
419#if HAVE_SYS_XATTR_H 532​#if HAVE_SYS_XATTR_H
420static bool isNtfsHidden(const QString &filename) 533​static bool isNtfsHidden(const QString &filename)
421{ 534​{
422 constexpr auto attrName = "system.ntfs_attrib_be"; 535​ constexpr auto attrName = "system.ntfs_attrib_be";
423 const auto filenameEncoded = QFile::encodeName(filename); 536​ const auto filenameEncoded = QFile::encodeName(filename);
424#ifdef Q_OS_MACOS 537​#ifdef Q_OS_MACOS
425 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0, 0, XATTR_NOFOLLOW); 538​ auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0, 0, XATTR_NOFOLLOW);
davidedmundsonUnsubmitted
Done

you need to check vallen for error returns.

davidedmundson: you need to check vallen for error returns.
426#else 539​#else
427 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0); 540​ auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0);
428#endif 541​#endif
429 if (length <= 0) { 542​ if (length <= 0) {
430 return false; 543​ return false;
431 } 544​ }
432 constexpr size_t xattr_size = 1024; 545​ constexpr size_t xattr_size = 1024;
433 char strAttr[xattr_size]; 546​ char strAttr[xattr_size];
▲ Show 20 Lines • Show All 443 Lines • ▼ Show 20 Line(s)953{
877 990​
878 auto reply = execAction.execute(); 991​ auto reply = execAction.execute();
879 if (reply->exec()) { 992​ if (reply->exec()) {
880 addTemporaryAuthorization(actionId); 993​ addTemporaryAuthorization(actionId);
881 return PrivilegeOperationReturnValue::success(); 994​ return PrivilegeOperationReturnValue::success();
882 } 995​ }
883 996​
884 return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED); 997​ return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED);
885} 998​}
ustaUnsubmitted
Not Done

isnt this ignoring acl_from_mode part ? I mean not sure but i think we need to check if it is nullptr or not before assigning it otherwise we will ignore the acl_from_mode part.

usta: isnt this ignoring acl_from_mode part ? I mean not sure but i think we need to check if it is…
arrowdAuthorUnsubmitted
Not Done

You seem to be right. I blamed the code down to svn->git import and this bug was present all the time.

To properly fix this I'd start with a test for KIO::chmod job that changes ACL permissions, but I can't test this on FreeBSD as we don't have acl_from_mode.

Anyways, it is out of scope of this review.

arrowd: You seem to be right. I blamed the code down to svn->git import and this bug was present all…