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

Changeset View

Standalone View

View Options

src/ioslaves/file/file_unix.cpp

Show All 33 Lines
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 41​#if HAVE_SYS_XATTR_H
42​#if defined(Q_OS_LINUX) || defined(__GLIBC__) || defined(Q_OS_MAC)
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
42#include <sys/xattr.h> 43​#include <sys/xattr.h>
44​#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
pinoUnsubmitted
Done

better check for sys/extattr.h instead

pino: better check for `sys/extattr.h` instead
45​#include <sys/extattr.h>
46​#endif
43#endif 47​#endif
44#include <utime.h> 48​#include <utime.h>
45 49​
46#include <KAuth> 50​#include <KAuth>
47#include <KRandom> 51​#include <KRandom>
48 52​
49#include "fdreceiver.h" 53​#include "fdreceiver.h"
50 54​
▲ Show 20 Lines • Show All 75 Lines • ▼ Show 20 Line(s)128{
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​
133void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, 137​void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl,
134 int _mode, JobFlags _flags) 138​ int _mode, JobFlags _flags)
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…
135{ 139​{
136 if (privilegeOperationUnitTestMode()) { 140​ if (privilegeOperationUnitTestMode()) {
dfaureUnsubmitted
Done

typo: soure => source

dfaure: typo: soure => source
137 finished(); 141​ finished();
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…
138 return; 142​ return;
brunsUnsubmitted
Done

just QByteArray keylist;

bruns: just `QByteArray keylist;`
139 } 143​ }
brunsUnsubmitted
Done

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

bruns: `while (true)` instead of `do {} while(true)`
140 144​
141 // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode; 145​ // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode;
brunsUnsubmitted
Done

extattr_list_fd, here and everywhere else

bruns: extattr_list_**fd**, here and everywhere else
142 146​
143 const QString src = srcUrl.toLocalFile(); 147​ const QString src = srcUrl.toLocalFile();
144 const QString dest = destUrl.toLocalFile(); 148​ const QString dest = destUrl.toLocalFile();
brunsUnsubmitted
Done

You never check if the source FS supports xattrs.

bruns: You never check if the source FS supports xattrs.
145 QByteArray _src(QFile::encodeName(src)); 149​ QByteArray _src(QFile::encodeName(src));
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.
146 QByteArray _dest(QFile::encodeName(dest)); 150​ QByteArray _dest(QFile::encodeName(dest));
147 151​
148 QT_STATBUF buff_src; 152​ QT_STATBUF buff_src;
149#if HAVE_POSIX_ACL 153​#if HAVE_POSIX_ACL
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 ...
150 acl_t acl; 154​ acl_t acl;
151#endif 155​#endif
brunsUnsubmitted
Done

This is still definitely wrong ...

bruns: This is still definitely wrong ...
152 if (QT_STAT(_src.data(), &buff_src) == -1) { 156​ if (QT_STAT(_src.data(), &buff_src) == -1) {
153 if (errno == EACCES) { 157​ if (errno == EACCES) {
154 error(KIO::ERR_ACCESS_DENIED, src); 158​ error(KIO::ERR_ACCESS_DENIED, src);
155 } else { 159​ } else {
156 error(KIO::ERR_DOES_NOT_EXIST, src); 160​ error(KIO::ERR_DOES_NOT_EXIST, src);
157 } 161​ }
158 return; 162​ return;
159 } 163​ }
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…
160 164​
brunsUnsubmitted
Done

"does not"

bruns: "does not"
161 if ((buff_src.st_mode & QT_STAT_MASK) == QT_STAT_DIR) { 165​ if ((buff_src.st_mode & QT_STAT_MASK) == QT_STAT_DIR) {
162 error(KIO::ERR_IS_DIRECTORY, src); 166​ error(KIO::ERR_IS_DIRECTORY, src);
dfaureUnsubmitted
Done

typo: srting => string

dfaure: typo: srting => string
163 return; 167​ return;
ustaUnsubmitted
Done

small typo in comment : itens => items

usta: small typo in comment : itens => items
164 } 168​ }
165 if (S_ISFIFO(buff_src.st_mode) || S_ISSOCK(buff_src.st_mode)) { 169​ if (S_ISFIFO(buff_src.st_mode) || S_ISSOCK(buff_src.st_mode)) {
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
166 error(KIO::ERR_CANNOT_OPEN_FOR_READING, src); 170​ error(KIO::ERR_CANNOT_OPEN_FOR_READING, src);
167 return; 171​ return;
dfaureUnsubmitted
Done

What's the difference with QByteArray key; ?

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

Should be true

bruns: Should be `true`
169 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.
170 QT_STATBUF buff_dest; 174​ QT_STATBUF buff_dest;
171 bool dest_exists = (QT_LSTAT(_dest.data(), &buff_dest) != -1); 175​ bool dest_exists = (QT_LSTAT(_dest.data(), &buff_dest) != -1);
brunsUnsubmitted
Done

why .squeeze() ?

bruns: why `.squeeze()` ?
172 if (dest_exists) { 176​ if (dest_exists) {
173 if (same_inode(buff_dest, buff_src)) { 177​ if (same_inode(buff_dest, buff_src)) {
brunsUnsubmitted
Done

"return a list of null terminated strings"

bruns: "return a list of null terminated strings"
174 error(KIO::ERR_IDENTICAL_FILES, dest); 178​ error(KIO::ERR_IDENTICAL_FILES, dest);
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…
175 return; 179​ return;
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.
176 } 180​ }
brunsUnsubmitted
Done

*keyPtr needs cast to unsigned char.

bruns: `*keyPtr` needs cast to unsigned char.
177 181​
178 if ((buff_dest.st_mode & QT_STAT_MASK) == QT_STAT_DIR) { 182​ if ((buff_dest.st_mode & QT_STAT_MASK) == QT_STAT_DIR) {
179 error(KIO::ERR_DIR_ALREADY_EXIST, dest); 183​ error(KIO::ERR_DIR_ALREADY_EXIST, dest);
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.
180 return; 184​ return;
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.
181 } 185​ }
brunsUnsubmitted
Done

"key size" or "size of the key"

bruns: "key size" or "size of the key"
182 186​
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…
183 if (!(_flags & KIO::Overwrite)) { 187​ if (!(_flags & KIO::Overwrite)) {
184 error(KIO::ERR_FILE_ALREADY_EXIST, dest); 188​ error(KIO::ERR_FILE_ALREADY_EXIST, dest);
185 return; 189​ return;
186 } 190​ }
187 191​
188 // If the destination is a symlink and overwrite is TRUE, 192​ // If the destination is a symlink and overwrite is TRUE,
189 // remove the symlink first to prevent the scenario where 193​ // remove the symlink first to prevent the scenario where
190 // the symlink actually points to current source! 194​ // the symlink actually points to current source!
191 if ((_flags & KIO::Overwrite) && ((buff_dest.st_mode & QT_STAT_MASK) == QT_STAT_LNK)) { 195​ if ((_flags & KIO::Overwrite) && ((buff_dest.st_mode & QT_STAT_MASK) == QT_STAT_LNK)) {
192 //qDebug() << "copy(): LINK DESTINATION"; 196​ //qDebug() << "copy(): LINK DESTINATION";
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.
193 if (!QFile::remove(dest)) { 197​ if (!QFile::remove(dest)) {
194 if (auto err = execWithElevatedPrivilege(DEL, {_dest}, errno)) { 198​ if (auto err = execWithElevatedPrivilege(DEL, {_dest}, errno)) {
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 ...
195 if (!err.wasCanceled()) { 199​ if (!err.wasCanceled()) {
196 error(KIO::ERR_CANNOT_DELETE_ORIGINAL, dest); 200​ error(KIO::ERR_CANNOT_DELETE_ORIGINAL, dest);
brunsUnsubmitted
Not Done

also wrong ...

bruns: also wrong ...
197 } 201​ }
198 return; 202​ return;
199 } 203​ }
200 } 204​ }
201 } 205​ }
202 } 206​ }
brunsUnsubmitted
Done

WRONG WRONG WRONG !!!

bruns: WRONG WRONG WRONG !!!
203 207​
204 QFile src_file(src); 208​ QFile src_file(src);
205 if (!src_file.open(QIODevice::ReadOnly)) { 209​ if (!src_file.open(QIODevice::ReadOnly)) {
brunsUnsubmitted
Done

No extra spaces for qDebug, inserts spaces itself.

bruns: No extra spaces for qDebug, inserts spaces itself.
206 if (auto err = tryOpen(src_file, _src, O_RDONLY, S_IRUSR, errno)) { 210​ if (auto err = tryOpen(src_file, _src, O_RDONLY, S_IRUSR, errno)) {
207 if (!err.wasCanceled()) { 211​ if (!err.wasCanceled()) {
208 error(KIO::ERR_CANNOT_OPEN_FOR_READING, src); 212​ error(KIO::ERR_CANNOT_OPEN_FOR_READING, src);
209 } 213​ }
210 return; 214​ return;
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…
211 } 215​ }
212 } 216​ }
213 217​
214#if HAVE_FADVISE 218​#if HAVE_FADVISE
215 posix_fadvise(src_file.handle(), 0, 0, POSIX_FADV_SEQUENTIAL); 219​ posix_fadvise(src_file.handle(), 0, 0, POSIX_FADV_SEQUENTIAL);
216#endif 220​#endif
217 221​
218 QFile dest_file(dest); 222​ QFile dest_file(dest);
219 if (!dest_file.open(QIODevice::Truncate | QIODevice::WriteOnly)) { 223​ if (!dest_file.open(QIODevice::Truncate | QIODevice::WriteOnly)) {
220 if (auto err = tryOpen(dest_file, _dest, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR, errno)) { 224​ if (auto err = tryOpen(dest_file, _dest, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR, errno)) {
221 if (!err.wasCanceled()) { 225​ if (!err.wasCanceled()) {
222 // qDebug() << "###### COULD NOT WRITE " << dest; 226​ // qDebug() << "###### COULD NOT WRITE " << dest;
brunsUnsubmitted
Done

"does not"

bruns: "does not"
223 if (err == EACCES) { 227​ if (err == EACCES) {
224 error(KIO::ERR_WRITE_ACCESS_DENIED, dest); 228​ error(KIO::ERR_WRITE_ACCESS_DENIED, dest);
225 } else { 229​ } else {
226 error(KIO::ERR_CANNOT_OPEN_FOR_WRITING, dest); 230​ error(KIO::ERR_CANNOT_OPEN_FOR_WRITING, dest);
227 } 231​ }
228 } 232​ }
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.
229 src_file.close(); 233​ src_file.close();
230 return; 234​ return;
231 } 235​ }
232 } 236​ }
233 237​
234 // nobody shall be allowed to peek into the file during creation 238​ // nobody shall be allowed to peek into the file during creation
235 // Note that error handling is omitted for this call, we don't want to error on e.g. VFAT 239​ // Note that error handling is omitted for this call, we don't want to error on e.g. VFAT
236 dest_file.setPermissions(QFileDevice::ReadOwner | QFileDevice::WriteOwner); 240​ dest_file.setPermissions(QFileDevice::ReadOwner | QFileDevice::WriteOwner);
▲ Show 20 Lines • Show All 180 Lines • ▼ Show 20 Line(s)
417} 421​}
418 422​
419#if HAVE_SYS_XATTR_H 423​#if HAVE_SYS_XATTR_H
420static bool isNtfsHidden(const QString &filename) 424​static bool isNtfsHidden(const QString &filename)
421{ 425​{
422 constexpr auto attrName = "system.ntfs_attrib_be"; 426​ constexpr auto attrName = "system.ntfs_attrib_be";
423 const auto filenameEncoded = QFile::encodeName(filename); 427​ const auto filenameEncoded = QFile::encodeName(filename);
424#ifdef Q_OS_MACOS 428​#ifdef Q_OS_MACOS
425 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0, 0, XATTR_NOFOLLOW); 429​ 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 430​#else
427 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0); 431​ auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0);
428#endif 432​#endif
429 if (length <= 0) { 433​ if (length <= 0) {
430 return false; 434​ return false;
431 } 435​ }
432 constexpr size_t xattr_size = 1024; 436​ constexpr size_t xattr_size = 1024;
433 char strAttr[xattr_size]; 437​ char strAttr[xattr_size];
▲ Show 20 Lines • Show All 443 Lines • ▼ Show 20 Line(s)844{
877 881​
878 auto reply = execAction.execute(); 882​ auto reply = execAction.execute();
879 if (reply->exec()) { 883​ if (reply->exec()) {
880 addTemporaryAuthorization(actionId); 884​ addTemporaryAuthorization(actionId);
881 return PrivilegeOperationReturnValue::success(); 885​ return PrivilegeOperationReturnValue::success();
882 } 886​ }
883 887​
884 return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED); 888​ return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED);
885} 889​}
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…