Differential D6197 Diff 20310 src/ioslaves/file/kauth/filehelper.cpp

Changeset View

Standalone View

View Options

src/ioslaves/file/kauth/filehelper.cpp

  • This file was added.
1​/***
2​ Copyright (C) 2017 by Chinmoy Ranjan Pradhan <chinmoyrp65@gmail.com>
3​
4​ This library is free software; you can redistribute it and/or
5​ modify it under the terms of the GNU Lesser General Public
6​ License as published by the Free Software Foundation; either
7​ version 2.1 of the License, or (at your option) version 3, or any
8​ later version accepted by the membership of KDE e.V. (or its
9​ successor approved by the membership of KDE e.V.), which shall
10​ act as a proxy defined in Section 6 of version 3 of the license.
11​
12​ This library is distributed in the hope that it will be useful,
13​ but WITHOUT ANY WARRANTY; without even the implied warranty of
14​ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15​ Lesser General Public License for more details.
16​
17​ You should have received a copy of the GNU Lesser General Public
18​ License along with this library. If not, see <http://www.gnu.org/licenses/>.
19​***/
20​
21​#include <utime.h>
22​#include <fcntl.h>
23​#include <unistd.h>
24​#include <dirent.h>
25​#include <sys/stat.h>
26​
27​#include "filehelper.h"
28​#include "fdsender.h"
29​#include "../file_p.h"
30​
31​static bool sendFileDescriptor(int fd, const char *socketPath)
32​{
33​ FdSender fdSender(socketPath);
34​ if (fdSender.isConnected() && fdSender.sendFileDescriptor(fd)) {
35​ return true;
dfaureUnsubmitted
Not Done

BTW what happens if two instances of this helper (and two instances of kio_file) try to do this at the same time? Don't they need different socket paths, to avoid interferring with each other?

I suppose the PID of the kio_file process should be added to this path, which means passing it in the QVariantMap to exec(), right?

dfaure: BTW what happens if two instances of this helper (and two instances of kio_file) try to do this…
36​ }
37​ return false;
38​}
39​
40​ActionReply FileHelper::exec(const QVariantMap &args)
dfaureUnsubmitted
Not Done

this basically abuses a global variable for something that could just be a return value from this function.

dfaure: this basically abuses a global variable for something that could just be a return value from…
41​{
42​ ActionReply reply;
dfaureUnsubmitted
Not Done

I assume this enum is duplicated elsewhere (in whoever is serializing the arguments for FileHelper), right?
Can the duplication be avoided by sharing a private header? Otherwise this at least needs a comment like "keep in sync with..." in both places.

dfaure: I assume this enum is duplicated elsewhere (in whoever is serializing the arguments for…
43​ QByteArray data = args["arguments"].toByteArray();
44​ QDataStream in(data);
45​ int action;
dfaureUnsubmitted
Not Done

should be unnecessary now, with the above suggested change

dfaure: should be unnecessary now, with the above suggested change
chinmoyrAuthorUnsubmitted
Not Done

In my system errno is set to 11. It seems like the case where function call succeeds but errno is set. But then it happens right after control reaches this method so I can't figure out which call might have set the error. I can't say if its unique to my system or not but there it is. So I think its best to set errno to 0 just to be on safe side.

chinmoyr: In my system errno is set to 11. It seems like the case where function call succeeds but errno…
46​ QVariant arg1, arg2, arg3, arg4;
47​ in >> action >> arg1 >> arg2 >> arg3 >> arg4;
dfaureUnsubmitted
Not Done

Well, this proves exactly my earlier point: we should only test errno *when* a libc function fails.

If this code is still checking errno after success somewhere, then *that* it what should be fixed. And once it's fixed, there's no need to reset errno here.

(BTW strerror(11) is EAGAIN, "Resource temporarily unavailable", rather frequent for non-blocking sockets, which is probably what triggered the slave to wake up in the first place.)

So, where is this code checking for errno even after success? In the caller of this method? It's hard to review all these separate review requests because I never have a global overview or the ability to search across the whole codebase -- but at the same time, everything in a single merge request would kill this slow webbrowser... [QtWebEngine compiled in debug mode] :-)

dfaure: Well, this proves exactly my earlier point: we should only test errno *when* a libc function…
chinmoyrAuthorUnsubmitted
Not Done

Actually action OPEN and OPENDIR were relying on this errno assignment (in the previous revision). I forgot that in this revision both of them return on success.
I have now removed that statement. Sorry for the trouble.

chinmoyr: Actually action OPEN and OPENDIR were relying on this errno assignment (in the previous…
48​
49​ // the path of an existing or a new file/dir upon which the method will operate
50​ const QByteArray path = arg1.toByteArray();
51​
52​ switch(action) {
53​ case CHMOD: {
54​ int mode = arg2.toInt();
55​ if (chmod(path.data(), mode) == 0) {
56​ return reply;
57​ }
58​ break;
59​ }
60​ case CHOWN: {
61​ int uid = arg2.toInt();
62​ int gid = arg3.toInt();
63​ if (chown(path.data(), uid, gid) == 0) {
64​ return reply;
65​ }
66​ break;
67​ }
68​ case DEL: {
69​ if (unlink(path.data()) == 0) {
70​ return reply;
71​ }
72​ break;
dfaureUnsubmitted
Not Done

this calls for a switch()

dfaure: this calls for a switch()
73​ }
74​ case MKDIR: {
75​ if (mkdir(path.data(), 0777) == 0) {
76​ return reply;
77​ }
78​ break;
79​ }
80​ case OPEN: {
81​ int oflags = arg2.toInt();
82​ int mode = arg3.toInt();
83​ int fd = open(path.data(), oflags, mode);
84​ bool success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData());
85​ close(fd);
86​ if (success) {
87​ return reply;
dfaureUnsubmitted
Not Done

opendir can return 0, in which case the next line will probably crash.

Overall, this whole method seriously lacks error handling in my opinion. Just checking for errno at the end isn't enough, e.g. in cases like this where multiple calls are being made.

And even otherwise, I'm unsure if ignoring the return value of chmod/chown/unlink/mkdir and *just* checking errno is enough or not. Does anyone else know?

dfaure: opendir can return 0, in which case the next line will probably crash. Overall, this whole…
chinmoyrAuthorUnsubmitted
Not Done

In case of OPEN and OPENDIR, I can see why we need error checking, opendir() can return null and there are some issues (I read on internet, can't recall the source) if close() fails. But in case of single function calls like chmod() I can't see how return value matters. Say if unlink fails for whatever reasons, we can return the error code and the checking for exact error code can be done by ioslave. Is there something I am overlooking?

chinmoyr: In case of OPEN and OPENDIR, I can see why we need error checking, opendir() can return null…
dfaureUnsubmitted
Not Done

The documentation for chown and others says:

Upon successful completion, these functions shall return 0. Otherwise, these functions shall return −1 and set errno to indicate the error.

It does NOT say, that errno isn't set when the function is successful. It seems to me that it would be perfectly valid for a libc implementation to do something like "try this, it fails (and sets errno), then try that, it worked, return 0".

For this reason I would feel much safer (especially in code run by root!) if the error handling was more classic, i.e. by checking return values.

dfaure: The documentation for chown and others says: Upon successful completion, these functions shall…
chinmoyrAuthorUnsubmitted
Not Done

It does NOT say, that errno isn't set when the function is successful. It seems to me that it would be perfectly valid for a libc implementation to do something like "try this, it fails (and sets errno), then try that, it worked, return 0".

It is valid and isn't that uncommon.

For this reason I would feel much safer (especially in code run by root!) if the error handling was more classic, i.e. by checking return values.

Currently helper sets the error and slave terminates due to Kauth::ErrorReply. Do you want this to not happen, like in case where chown succeeds but errno is set? And what error code should I check for and whether it should be for each case or at the end of method?

chinmoyr: >It does NOT say, that errno isn't set when the function is successful. It seems to me that it…
88​ }
89​ break;
90​ }
91​ case OPENDIR: {
92​ DIR *dp = opendir(path.data());
93​ bool success = false;
94​ if (dp) {
95​ int fd = dirfd(dp);
96​ success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData());
97​ closedir(dp);
98​ if (success) {
99​ return reply;
100​ }
101​ }
102​ break;
103​ }
dfaureUnsubmitted
Not Done

missing else or break here, no?
You're sending the valid fd, followed by -1.

dfaure: missing else or break here, no? You're sending the valid fd, followed by -1.
104​ case RENAME: {
105​ const QByteArray newName = arg2.toByteArray();
dfaureUnsubmitted
Not Done

You send fd=-1 if opendir fails, but not if dirfd fails.
Is it even needed to send -1, if we're going to return an error reply anyway?
(disclaimer, I don't know sendFileDescriptor nor what the code on the calling side does)

dfaure: You send fd=-1 if opendir fails, but not if dirfd fails. Is it even needed to send -1, if we're…
106​ if (rename(path.data(), newName.data()) == 0) {
107​ return reply;
108​ }
109​ break;
110​ }
111​ case RMDIR: {
112​ if (rmdir(path.data()) == 0) {
113​ return reply;
114​ }
115​ break;
116​ }
117​ case SYMLINK: {
118​ const QByteArray target = arg2.toByteArray();
119​ if (symlink(target.data(), path.data()) == 0) {
120​ return reply;
121​ }
122​ break;
123​ }
124​ case UTIME: {
125​ utimbuf ut;
126​ ut.actime = arg2.toULongLong();
127​ ut.modtime = arg3.toULongLong();
128​ if (utime(path.data(), &ut) == 0) {
129​ return reply;
130​ }
131​ break;
132​ }
133​ };
134​
135​ reply.setError(errno ? errno : -1);
dfaureUnsubmitted
Not Done

I hope we never have the opposite problem, where a libc function returns non-zero but "forgets" to set errno.
Now that you added return statements on success everywhere, maybe this line could say Q_ASSERT(errno != 0), or

reply.setError(errno ? errno : -1);

to make sure we never return success when an error happened.

dfaure: I hope we never have the opposite problem, where a libc function returns non-zero but "forgets"…
136​ return reply;
137​}
138​
139​KAUTH_HELPER_MAIN("org.kde.kio.file", FileHelper)