D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200417/6024ece9/attachment.html>