Differential D17816 Diff 77804 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​
64​
60using namespace KIO; 65​using namespace KIO;
61 66​
62#define MAX_IPC_SIZE (1024*32) 67​#define MAX_IPC_SIZE (1024*32)
63 68​
64static bool 69​static bool
65same_inode(const QT_STATBUF &src, const QT_STATBUF &dest) 70​same_inode(const QT_STATBUF &src, const QT_STATBUF &dest)
66{ 71​{
67 if (src.st_ino == dest.st_ino && 72​ if (src.st_ino == dest.st_ino &&
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Line(s)129{
125 KAuth::Action execAction(QStringLiteral("org.kde.kio.file.exec")); 130​ KAuth::Action execAction(QStringLiteral("org.kde.kio.file.exec"));
126 execAction.setHelperId(QStringLiteral("org.kde.kio.file")); 131​ execAction.setHelperId(QStringLiteral("org.kde.kio.file"));
127 if (execAction.status() == KAuth::Action::AuthorizedStatus) { 132​ if (execAction.status() == KAuth::Action::AuthorizedStatus) {
128 return execWithElevatedPrivilege(action, args, errcode); 133​ return execWithElevatedPrivilege(action, args, errcode);
129 } 134​ }
130 return PrivilegeOperationReturnValue::failure(errcode); 135​ return PrivilegeOperationReturnValue::failure(errcode);
131} 136​}
132 137​
138​
139​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…
140​{
141​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr)
dfaureUnsubmitted
Done

typo: soure => source

dfaure: typo: soure => source
142​ ssize_t listlen = flistxattr(src_fd, nullptr, 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…
143​#elif defined(Q_OS_MAC)
brunsUnsubmitted
Done

just QByteArray keylist;

bruns: just `QByteArray keylist;`
144​ ssize_t listlen = flistxattr(src_fd, nullptr, 0, 0);
brunsUnsubmitted
Done

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

bruns: `while (true)` instead of `do {} while(true)`
145​#elif HAVE_SYS_EXTATTR
146​ ssize_t listlen = extattr_list_file(src_fd, EXTATTR_NAMESPACE_USER, nullptr, 0);
brunsUnsubmitted
Done

extattr_list_fd, here and everywhere else

bruns: extattr_list_**fd**, here and everywhere else
147​#endif
148​ if (listlen == -1) {
149​ qCDebug(KIO_FILE) << "libc failed to extract list of xattr from file";
brunsUnsubmitted
Done

You never check if the source FS supports xattrs.

bruns: You never check if the source FS supports xattrs.
150​ return false;
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.
151​ }
152​ if (listlen == 0) {
153​ qCDebug(KIO_FILE) << "file " << src_fd << " don't have any xattr";
154​ return false;
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 ...
155​ }
156​ QByteArray keylist(listlen, Qt::Uninitialized);
brunsUnsubmitted
Done

This is still definitely wrong ...

bruns: This is still definitely wrong ...
157​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr)
158​ listlen = flistxattr(src_fd, keylist.data(), listlen);
159​#elif defined(Q_OS_MAC)
160​ listlen = flistxattr(src_fd, keylist.data(), listlen, 0);
161​#elif HAVE_SYS_EXTATTR
162​ listlen = extattr_list_file(src_fd, EXTATTR_NAMESPACE_USER, keylist.data(), listlen);
163​#endif
164​ QList<QByteArray> m_keyList = keylist.split('\0');
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…
165​ if (m_keyList.last().isEmpty()) m_keyList.removeLast(); // the last item may be empty
brunsUnsubmitted
Done

"does not"

bruns: "does not"
166​ // for each key
167​ for (const auto & key : qAsConst(m_keyList)) {
dfaureUnsubmitted
Done

typo: srting => string

dfaure: typo: srting => string
168​ // get the size of value
ustaUnsubmitted
Done

small typo in comment : itens => items

usta: small typo in comment : itens => items
169​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr)
170​ ssize_t valuelen = fgetxattr(src_fd, key.constData(), nullptr, 0);
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
171​#elif defined(Q_OS_MAC)
172​ ssize_t valuelen = fgetxattr(src_fd, key.constData(), nullptr, 0, 0, 0);
dfaureUnsubmitted
Done

What's the difference with QByteArray key; ?

dfaure: What's the difference with `QByteArray key;` ?
173​#elif HAVE_SYS_EXTATTR
brunsUnsubmitted
Done

Should be true

bruns: Should be `true`
174​ ssize_t valuelen = extattr_get_file(src_fd, EXTATTR_NAMESPACE_USER, key.constData(), nullptr, 0);
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.
175​#endif
176​ if (valuelen == -1) {
brunsUnsubmitted
Done

why .squeeze() ?

bruns: why `.squeeze()` ?
177​ qCWarning(KIO_FILE) << "libc failed to extract a xattr value from file";
178​ continue;
brunsUnsubmitted
Done

"return a list of null terminated strings"

bruns: "return a list of null terminated strings"
179​ }
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…
180​ QByteArray value(valuelen + 1, Qt::Uninitialized);
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.
181​ // get the value for key
brunsUnsubmitted
Done

*keyPtr needs cast to unsigned char.

bruns: `*keyPtr` needs cast to unsigned char.
182​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr)
183​ valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen);
184​#elif defined(Q_OS_MAC)
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.
185​ vallen = fgetxattr(src_fd, key.constData(), value.data(), valuelen, 0, 0);
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.
186​#elif HAVE_SYS_EXTATTR
brunsUnsubmitted
Done

"key size" or "size of the key"

bruns: "key size" or "size of the key"
187​ vallen = extattr_get_file(src_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.data(), valuelen);
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…
188​#endif
189​ //write key:value pair on dest file
190​#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr)
191​ ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.data(), valuelen, 0);
192​#elif defined(Q_OS_MAC)
193​ ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.data(), valuelen, 0, 0);
194​#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
195​ ssize_t destlen = extattr_set_file(dest_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.data(), valuelen);
196​#endif
197​ if (destlen == -1) {
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.
198​ if (errno == ENOTSUP) {
199​ qCWarning(KIO_FILE) << "destination filesystem don't support xattrs";
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 ...
200​ } else {
201​ qCWarning(KIO_FILE) << "failed to write a xattr on destination file";
brunsUnsubmitted
Not Done

also wrong ...

bruns: also wrong ...
202​ }
203​ }
204​ }
205​ return true;
206​}
207​
brunsUnsubmitted
Done

WRONG WRONG WRONG !!!

bruns: WRONG WRONG WRONG !!!
133void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, 208​void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl,
134 int _mode, JobFlags _flags) 209​ int _mode, JobFlags _flags)
135{ 210​{
brunsUnsubmitted
Done

No extra spaces for qDebug, inserts spaces itself.

bruns: No extra spaces for qDebug, inserts spaces itself.
136 if (privilegeOperationUnitTestMode()) { 211​ if (privilegeOperationUnitTestMode()) {
137 finished(); 212​ finished();
138 return; 213​ return;
139 } 214​ }
140 215​
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…
141 // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode; 216​ // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode;
142 217​
143 const QString src = srcUrl.toLocalFile(); 218​ const QString src = srcUrl.toLocalFile();
144 const QString dest = destUrl.toLocalFile(); 219​ const QString dest = destUrl.toLocalFile();
145 QByteArray _src(QFile::encodeName(src)); 220​ QByteArray _src(QFile::encodeName(src));
146 QByteArray _dest(QFile::encodeName(dest)); 221​ QByteArray _dest(QFile::encodeName(dest));
147 222​
148 QT_STATBUF buff_src; 223​ QT_STATBUF buff_src;
149#if HAVE_POSIX_ACL 224​#if HAVE_POSIX_ACL
150 acl_t acl; 225​ acl_t acl;
151#endif 226​#endif
152 if (QT_STAT(_src.data(), &buff_src) == -1) { 227​ if (QT_STAT(_src.data(), &buff_src) == -1) {
brunsUnsubmitted
Done

"does not"

bruns: "does not"
153 if (errno == EACCES) { 228​ if (errno == EACCES) {
154 error(KIO::ERR_ACCESS_DENIED, src); 229​ error(KIO::ERR_ACCESS_DENIED, src);
155 } else { 230​ } else {
156 error(KIO::ERR_DOES_NOT_EXIST, src); 231​ error(KIO::ERR_DOES_NOT_EXIST, src);
157 } 232​ }
158 return; 233​ return;
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.
159 } 234​ }
160 235​
161 if ((buff_src.st_mode & QT_STAT_MASK) == QT_STAT_DIR) { 236​ if ((buff_src.st_mode & QT_STAT_MASK) == QT_STAT_DIR) {
162 error(KIO::ERR_IS_DIRECTORY, src); 237​ error(KIO::ERR_IS_DIRECTORY, src);
163 return; 238​ return;
164 } 239​ }
165 if (S_ISFIFO(buff_src.st_mode) || S_ISSOCK(buff_src.st_mode)) { 240​ if (S_ISFIFO(buff_src.st_mode) || S_ISSOCK(buff_src.st_mode)) {
166 error(KIO::ERR_CANNOT_OPEN_FOR_READING, src); 241​ error(KIO::ERR_CANNOT_OPEN_FOR_READING, src);
▲ Show 20 Lines • Show All 75 Lines • ▼ Show 20 Line(s)
242#if HAVE_POSIX_ACL 317​#if HAVE_POSIX_ACL
243 acl = acl_get_fd(src_file.handle()); 318​ acl = acl_get_fd(src_file.handle());
244 if (acl && !isExtendedACL(acl)) { 319​ if (acl && !isExtendedACL(acl)) {
245 // qDebug() << _dest.data() << " doesn't have extended ACL"; 320​ // qDebug() << _dest.data() << " doesn't have extended ACL";
246 acl_free(acl); 321​ acl_free(acl);
247 acl = nullptr; 322​ acl = nullptr;
248 } 323​ }
249#endif 324​#endif
325​
250 totalSize(buff_src.st_size); 326​ totalSize(buff_src.st_size);
251 327​
252 KIO::filesize_t processed_size = 0; 328​ KIO::filesize_t processed_size = 0;
253 char buffer[ MAX_IPC_SIZE ]; 329​ char buffer[ MAX_IPC_SIZE ];
254 ssize_t n = 0; 330​ ssize_t n = 0;
255#ifdef USE_SENDFILE 331​#ifdef USE_SENDFILE
256 bool use_sendfile = buff_src.st_size < 0x7FFFFFFF; 332​ bool use_sendfile = buff_src.st_size < 0x7FFFFFFF;
257#endif 333​#endif
▲ Show 20 Lines • Show All 66 Lines • ▼ Show 20 Line(s)395#endif
324 } 400​ }
325 processed_size += n; 401​ processed_size += n;
326#ifdef USE_SENDFILE 402​#ifdef USE_SENDFILE
327 } 403​ }
328#endif 404​#endif
329 processedSize(processed_size); 405​ processedSize(processed_size);
330 } 406​ }
331 407​
408​ // Copy Extended attributes
409​#if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H
410​ if (!copyXattrs(src_file.handle(), dest_file.handle())) {
411​ qCDebug(KIO_FILE) << "cant copy Extended attributes";
412​ }
413​#endif
414​
332 src_file.close(); 415​ src_file.close();
333 dest_file.close(); 416​ dest_file.close();
334 417​
335 if (dest_file.error() != QFile::NoError) { 418​ if (dest_file.error() != QFile::NoError) {
336 qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString(); 419​ qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString();
337 error(KIO::ERR_CANNOT_WRITE, dest); 420​ error(KIO::ERR_CANNOT_WRITE, dest);
338#if HAVE_POSIX_ACL 421​#if HAVE_POSIX_ACL
339 if (acl) { 422​ if (acl) {
▲ Show 20 Lines • Show All 77 Lines • ▼ Show 20 Line(s)
417} 500​}
418 501​
419#if HAVE_SYS_XATTR_H 502​#if HAVE_SYS_XATTR_H
420static bool isNtfsHidden(const QString &filename) 503​static bool isNtfsHidden(const QString &filename)
421{ 504​{
422 constexpr auto attrName = "system.ntfs_attrib_be"; 505​ constexpr auto attrName = "system.ntfs_attrib_be";
423 const auto filenameEncoded = QFile::encodeName(filename); 506​ const auto filenameEncoded = QFile::encodeName(filename);
424#ifdef Q_OS_MACOS 507​#ifdef Q_OS_MACOS
425 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0, 0, XATTR_NOFOLLOW); 508​ 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 509​#else
427 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0); 510​ auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0);
428#endif 511​#endif
429 if (length <= 0) { 512​ if (length <= 0) {
430 return false; 513​ return false;
431 } 514​ }
432 constexpr size_t xattr_size = 1024; 515​ constexpr size_t xattr_size = 1024;
433 char strAttr[xattr_size]; 516​ char strAttr[xattr_size];
▲ Show 20 Lines • Show All 443 Lines • ▼ Show 20 Line(s)923{
877 960​
878 auto reply = execAction.execute(); 961​ auto reply = execAction.execute();
879 if (reply->exec()) { 962​ if (reply->exec()) {
880 addTemporaryAuthorization(actionId); 963​ addTemporaryAuthorization(actionId);
881 return PrivilegeOperationReturnValue::success(); 964​ return PrivilegeOperationReturnValue::success();
882 } 965​ }
883 966​
884 return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED); 967​ return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED);
885} 968​}
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…