maltek requested changes to this revision.
maltek added a comment.
This revision now requires changes to proceed.
I noticed a few more things on the second read.
INLINE COMMENTS
> filehelper.cpp:123
> + const QByteArray baseName = basename(tempPath2.data());
> + int parent_fd = open(parentDir.data(), O_DIRECTORY | O_PATH |
> O_NOFOLLOW);
> + int base_fd = -1;
This needs error handling.
> filehelper.cpp:129
> +
> + if (action != CHMOD & action != CHOWN && action != UTIME) {
> + targetPrivilege = getTargetPrivilege(parent_fd);
typo: there's a second & missing after the first condition. (I don't think it
ends up affecting the result.)
> filehelper.cpp:132
> + } else {
> + base_fd = openat(parent_fd, baseName.data(), O_NOFOLLOW);
> + targetPrivilege = getTargetPrivilege(base_fd);
There's no error handling here, which will likely lead to weird `EBADF` errors
getting returned later.
> filehelper.cpp:133
> + base_fd = openat(parent_fd, baseName.data(), O_NOFOLLOW);
> + targetPrivilege = getTargetPrivilege(base_fd);
> }
For `chown`, dropping privileges here means that the `chown` later can't
succeed - it's not possible to 'gift' a file to another user. I think it should
be handled more like `DEL/RMDIR/MKDIR` etc.
> filehelper.cpp:150
> + int gid = arg3.toInt();
> + if (fchown(base_fd, uid, gid) == -1) {
> + reply.setError(errno);
I just realized that this wouldn't allow changing the owner of symbolic links.
The way to go here is `lchown`.
> filehelper.cpp:187
> + gainPrivilege(origPrivilege);
> + bool sendSuccess = sendFileDescriptor(fd,
> arg4.toByteArray().constData());
> + if (fd != -1 && sendSuccess) {
In the error case, this attempts sending fd `-1`. I haven't checked the
underlying code, but this will probably pollute `errno` with something
unrelated to the underlying error.
> filehelper.cpp:209
> + if (symlinkat(target.data(), parent_fd, baseName.data()) ==
> -1) {
> + return reply;
> + }
This early return skips all the deintialization code in the end of the
function. Shouldn't it just be `reply.setError(errno);` like for all the other
operations?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D14467
To: chinmoyr, dfaure, ngraham, elvisangelaccio, #frameworks, #dolphin, maltek
Cc: maltek, mreeves, mgerstner, fvogt, kde-frameworks-devel, LeGast00n,
michaelh, ngraham, bruns
