bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> file_unix.cpp:141
> + // Get the list of keys
> + ssize_t listlen = 512;
> + QByteArray keylist(listlen, Qt::Uninitialized);
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.
> file_unix.cpp:153
> + if (errno == ERANGE) {
> + keylist.resize(listlen + 512);
> + }
as listlen is -1 here, you always resize to 511 ...
> file_unix.cpp:163
> + }
> + } while (listlen == -1 && errno == ERANGE);
> +
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.
> file_unix.cpp:173
> + // For each key
> + keylist.squeeze();
> + while (keyPtr != keylist.cend()) {
Thats bad. Thats really bad.
squeeze may reallocate. This invalidates keyPtr.
> file_unix.cpp:186
> + ssize_t valuelen = 512;
> + QByteArray value(valuelen, Qt::Uninitialized);
> + do {
`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)`.
> file_unix.cpp:196
> + if (valuelen == -1 && errno == ERANGE) {
> + value.resize(valuelen + 512);
> + }
-1 + 512 = 511 ...
Should be `value.resize(value.size() + 512)` or something similar.
> file_unix.cpp:198
> + }
> + } while (valuelen == -1 && errno == ERANGE);
> +
Again, valuelen may be -1 here ...
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17816
To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho,
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane,
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh
