Changeset View
Standalone View
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> | ||
pino: `HAVE_SYS_XATTR_H` from `config-kioslave-file.h` can be used here | |||||
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 | | ||||
60 | using namespace KIO; | 64 | using namespace KIO; | ||
61 | 65 | | |||
62 | #define MAX_IPC_SIZE (1024*32) | 66 | #define MAX_IPC_SIZE (1024*32) | ||
63 | 67 | | |||
64 | static bool | 68 | static bool | ||
65 | same_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) | ||||
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). 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 | ||||
dfaure: typo: soure => source | |||||
141 | ssize_t listlen = 0; | ||||
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(listlen, Qt::Uninitialized); | ||||
bruns: just `QByteArray keylist;` | |||||
143 | do { | ||||
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) | ||||
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); | ||||
bruns: You never check if the source FS supports xattrs. | |||||
149 | #elif HAVE_SYS_EXTATTR_H | ||||
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; | ||||
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 | |||||
bruns: as listlen is -1 here, you always resize to 511 ... | |||||
154 | } | ||||
155 | if (listlen > 0 && keylist.size() > 0) { | ||||
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 && errno == ENOTSUP) { | ||||
163 | qCDebug(KIO_FILE) << "source filesystem don't support xattrs"; | ||||
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… | |||||
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… | |||||
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… | |||||
Not because if the file don't have a xattr the function returns on 153. The returned keylist should have at least one key. 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… | |||||
This is wrong for the BSD implementation:
bruns: This is wrong for the BSD implementation:
> extattr_list_file() returns a list of attributes… | |||||
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. cochise: Well... This will need some platform specif code to read the buffer and place in the m_keyList… | |||||
man 2 flistxattr:
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 | return false; | ||||
bruns: "does not" | |||||
165 | } | ||||
166 | if (listlen == 0) { | ||||
dfaure: typo: srting => string | |||||
167 | qCDebug(KIO_FILE) << "file" << src_fd << "don't have any xattr"; | ||||
usta: small typo in comment : itens => items | |||||
168 | return false; | ||||
169 | } | ||||
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… | |||||
bruns: `src_fd` is quite meaningless as debug output | |||||
170 | } while (true); | ||||
171 | | ||||
dfaure: What's the difference with `QByteArray key;` ? | |||||
172 | keylist.resize(listlen); | ||||
bruns: Should be `true` | |||||
173 | keylist.squeeze(); | ||||
bruns: Thats bad. Thats really bad.
squeeze may reallocate. This invalidates keyPtr. | |||||
174 | | ||||
175 | // Linux and MacOS return = list of null terminated string, each string = [data,'\0'] | ||||
bruns: why `.squeeze()` ? | |||||
176 | // BSD return = list of items, each item prepended of 1 byte size = [size, data] | ||||
177 | QByteArray::const_iterator keyPtr = keylist.cbegin(); | ||||
bruns: "return a list of null terminated strings" | |||||
178 | size_t keyLen; | ||||
"BSDs return a list of items, ..." or "BSD returns a list of items, ..." bruns: "BSDs return a list of items, ..." or "BSD returns a list of items, ..."
"..., each item… | |||||
179 | QByteArray key; | ||||
bruns: Allocate outside the loop | |||||
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… | |||||
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. | |||||
cochise: This look really better. | |||||
180 | QByteArray value; | ||||
bruns: `*keyPtr` needs cast to unsigned char. | |||||
181 | | ||||
182 | // For each key | ||||
183 | while (keyPtr != keylist.cend()) { | ||||
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… | |||||
cochise: Removed clear, as is redundant. | |||||
184 | // Get the size of key | ||||
bruns: valuelen | |||||
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. dfaure: You're doing two copies here. From `offset` to `key.data()`, and then from `key.data()` -- the… | |||||
So many time looking for a way to get a copy until a null, and it's in one of the constructors. cochise: So many time looking for a way to get a copy until a null, and it's in one of the constructors. | |||||
185 | #if HAVE_SYS_XATTR_H | ||||
bruns: "key size" or "size of the key" | |||||
186 | keyLen = strlen(keyPtr); | ||||
bruns: valuelen | |||||
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… | |||||
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 | #elif HAVE_SYS_EXTATTR_H | ||||
188 | keyLen = static_cast<unsigned char>(*keyPtr); | ||||
189 | keyPtr++; | ||||
190 | #endif | ||||
191 | QByteArray key(keyPtr, keyLen); | ||||
192 | | ||||
193 | // Get the value for key | ||||
194 | ssize_t valuelen = 512; | ||||
195 | QByteArray value(valuelen, Qt::Uninitialized); | ||||
196 | do { | ||||
-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 | #if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC) | ||||
198 | valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen); | ||||
bruns: you can return here, no value in trying again and again ... | |||||
bruns: Again, valuelen may be -1 here ... | |||||
199 | #elif defined(Q_OS_MAC) | ||||
200 | valuelen = fgetxattr(src_fd, key.constData(), value.data(), valuelen, 0, 0); | ||||
bruns: also wrong ... | |||||
201 | #elif HAVE_SYS_EXTATTR_H | ||||
202 | valuelen = extattr_get_fd(src_fd, EXTATTR_NAMESPACE_USER, key.constData(), NULL, 0); | ||||
203 | extattr_get_fd(src_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.data(), valuelen); | ||||
204 | #endif | ||||
205 | if (valuelen == -1 && errno == ERANGE) { | ||||
206 | value.resize(valuelen + 512); | ||||
bruns: WRONG WRONG WRONG !!! | |||||
207 | } | ||||
208 | } while (valuelen == -1 && errno == ERANGE); | ||||
209 | | ||||
bruns: No extra spaces for qDebug, inserts spaces itself. | |||||
210 | // Write key:value pair on destination | ||||
211 | #if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC) | ||||
212 | ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.constData(), valuelen, 0); | ||||
213 | #elif defined(Q_OS_MAC) | ||||
214 | ssize_t destlen = fsetxattr(dest_fd, key.constData(), value.constData(), valuelen, 0, 0); | ||||
bruns: Infinite loop on valuelen == 0 | |||||
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… | |||||
"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 | #elif HAVE_SYS_EXTATTR_H | ||||
216 | ssize_t destlen = extattr_set_fd(dest_fd, EXTATTR_NAMESPACE_USER, key.constData(), value.constData(), valuelen); | ||||
217 | #endif | ||||
218 | if (destlen == -1 && errno == ENOTSUP) { | ||||
219 | qCDebug(KIO_FILE) << "Destination filesystem don't support xattrs"; | ||||
220 | return false; | ||||
221 | } | ||||
222 | | ||||
223 | #if HAVE_SYS_XATTR_H | ||||
224 | keyPtr += keyLen + 1; | ||||
225 | #elif HAVE_SYS_EXTATTR_H | ||||
226 | keyPtr += keyLen; | ||||
bruns: "does not" | |||||
227 | #endif | ||||
228 | } | ||||
229 | return true; | ||||
230 | } | ||||
231 | | ||||
133 | void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, | 232 | void FileProtocol::copy(const QUrl &srcUrl, const QUrl &destUrl, | ||
bruns: what about `#elif defined(Q_OS_MAC)`? | |||||
@bruns HAVE_SYS_XATTR_H covers mac and non-mac here. dfaure: @bruns HAVE_SYS_XATTR_H covers mac and non-mac here. | |||||
134 | int _mode, JobFlags _flags) | 233 | int _mode, JobFlags _flags) | ||
135 | { | 234 | { | ||
136 | if (privilegeOperationUnitTestMode()) { | 235 | if (privilegeOperationUnitTestMode()) { | ||
137 | finished(); | 236 | finished(); | ||
138 | return; | 237 | return; | ||
139 | } | 238 | } | ||
140 | 239 | | |||
141 | // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode; | 240 | // qDebug() << "copy(): " << srcUrl << " -> " << destUrl << ", mode=" << _mode; | ||
▲ Show 20 Lines • Show All 182 Lines • ▼ Show 20 Line(s) | 418 | #endif | |||
324 | } | 423 | } | ||
325 | processed_size += n; | 424 | processed_size += n; | ||
326 | #ifdef USE_SENDFILE | 425 | #ifdef USE_SENDFILE | ||
327 | } | 426 | } | ||
328 | #endif | 427 | #endif | ||
329 | processedSize(processed_size); | 428 | processedSize(processed_size); | ||
330 | } | 429 | } | ||
331 | 430 | | |||
431 | // Copy Extended attributes | ||||
432 | #if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H | ||||
433 | if (!copyXattrs(src_file.handle(), dest_file.handle())) { | ||||
434 | qCDebug(KIO_FILE) << "cant copy Extended attributes"; | ||||
435 | } | ||||
436 | #endif | ||||
437 | | ||||
332 | src_file.close(); | 438 | src_file.close(); | ||
333 | dest_file.close(); | 439 | dest_file.close(); | ||
334 | 440 | | |||
335 | if (dest_file.error() != QFile::NoError) { | 441 | if (dest_file.error() != QFile::NoError) { | ||
336 | qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString(); | 442 | qCWarning(KIO_FILE) << "Error when closing file descriptor[2]:" << dest_file.errorString(); | ||
337 | error(KIO::ERR_CANNOT_WRITE, dest); | 443 | error(KIO::ERR_CANNOT_WRITE, dest); | ||
338 | #if HAVE_POSIX_ACL | 444 | #if HAVE_POSIX_ACL | ||
339 | if (acl) { | 445 | if (acl) { | ||
▲ Show 20 Lines • Show All 77 Lines • ▼ Show 20 Line(s) | |||||
417 | } | 523 | } | ||
418 | 524 | | |||
419 | #if HAVE_SYS_XATTR_H | 525 | #if HAVE_SYS_XATTR_H | ||
420 | static bool isNtfsHidden(const QString &filename) | 526 | static bool isNtfsHidden(const QString &filename) | ||
421 | { | 527 | { | ||
422 | constexpr auto attrName = "system.ntfs_attrib_be"; | 528 | constexpr auto attrName = "system.ntfs_attrib_be"; | ||
423 | const auto filenameEncoded = QFile::encodeName(filename); | 529 | const auto filenameEncoded = QFile::encodeName(filename); | ||
424 | #ifdef Q_OS_MACOS | 530 | #ifdef Q_OS_MACOS | ||
425 | auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0, 0, XATTR_NOFOLLOW); | 531 | auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0, 0, XATTR_NOFOLLOW); | ||
davidedmundson: you need to check vallen for error returns.
| |||||
426 | #else | 532 | #else | ||
427 | auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0); | 533 | auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0); | ||
428 | #endif | 534 | #endif | ||
429 | if (length <= 0) { | 535 | if (length <= 0) { | ||
430 | return false; | 536 | return false; | ||
431 | } | 537 | } | ||
432 | constexpr size_t xattr_size = 1024; | 538 | constexpr size_t xattr_size = 1024; | ||
433 | char strAttr[xattr_size]; | 539 | char strAttr[xattr_size]; | ||
▲ Show 20 Lines • Show All 443 Lines • ▼ Show 20 Line(s) | 946 | { | |||
877 | 983 | | |||
878 | auto reply = execAction.execute(); | 984 | auto reply = execAction.execute(); | ||
879 | if (reply->exec()) { | 985 | if (reply->exec()) { | ||
880 | addTemporaryAuthorization(actionId); | 986 | addTemporaryAuthorization(actionId); | ||
881 | return PrivilegeOperationReturnValue::success(); | 987 | return PrivilegeOperationReturnValue::success(); | ||
882 | } | 988 | } | ||
883 | 989 | | |||
884 | return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED); | 990 | return PrivilegeOperationReturnValue::failure(KIO::ERR_ACCESS_DENIED); | ||
885 | } | 991 | } | ||
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… | |||||
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… |
HAVE_SYS_XATTR_H from config-kioslave-file.h can be used here