D6197: Add kauth helper to file ioslave
This revision was automatically updated to reflect the committed changes. Closed by commit R241:17bf6df0d8ba: Add kauth helper to file ioslave (authored by chinmoyr). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=20608=25217 REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr reopened this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
This revision was automatically updated to reflect the committed changes. Closed by commit R241:29a741982fe5: Add kauth helper to file ioslave (authored by chinmoyr). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D6197?vs=20310=20608#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=20310=20608 REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure accepted this revision. BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 20310.
chinmoyr added a comment.
replaced occurence of sharefd.{h, cpp} with fdsender.{h, cpp}
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6197?vs=18504=20310
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6197
AFFECTED FILES
src/ioslaves/file/CMakeLists.txt
src/ioslaves/file/file_p.h
src/ioslaves/file/kauth/CMakeLists.txt
src/ioslaves/file/kauth/file.actions
src/ioslaves/file/kauth/filehelper.cpp
src/ioslaves/file/kauth/filehelper.h
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure added a comment. (strange that Phabricator still says "Needs Review" when this has two approvals) REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
elvisangelaccio accepted this revision as: elvisangelaccio. elvisangelaccio added inline comments. INLINE COMMENTS > file.actions:3 > +Name=Execute action as root. > +Description=Root privileges are required to complete the action. > +Policy=auth_admin We could use more descriptive sentences here. Note also that we are not necessarily asking for the root password. How about something like: - Name=File Operation - Description=The file operation that you requested requires elevated privileges REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure accepted this revision. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 18504. chinmoyr added a comment. - Minor changes in sendFileDescriptor function because constructor of FdSender changed in https://phabricator.kde.org/D6709. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=18301=18504 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 18301. chinmoyr added a comment. -Removed the hard-coded socket path. Now it will take socket path from ioslave. -Removed sendFileDescriptor(-1) calls. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=18300=18301 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 18300. chinmoyr added a comment. - Removed the hard-coded socket path. Now it will take socket path from ioslave. - Removed sendFileDescriptor(-1) calls. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=18235=18300 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/sharefd.cpp src/ioslaves/file/sharefd.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added a comment. > Is it even needed to send -1, if we're going to return an error reply anyway? The current logic is "after a successful sendFileDescriptor close the socket". So sending -1 was to ensure socket is closed. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filehelper.cpp:34
> +FdSender fdSender;
> +fdSender.connectToPath("org_kde_kio_file_helper_socket");
> +if (fdSender.isConnected()) {
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?
> filehelper.cpp:104
> +} else {
> +sendFileDescriptor(-1);
> +}
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)
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added inline comments. INLINE COMMENTS > dfaure wrote in filehelper.cpp:46 > 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] :-) 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. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 18235. chinmoyr added a comment. - Remove errno assignment CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=18201=18235 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > filehelper.cpp:46 > +// Set it to 0. > +errno = 0; > + 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] :-) REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added inline comments. INLINE COMMENTS > dfaure wrote in filehelper.cpp:44 > should be unnecessary now, with the above suggested change 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. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 18201. chinmoyr added a comment. - fixed minor issues CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=18012=18201 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filehelper.cpp:39
> +}
> +errno = -1;
> +}
this basically abuses a global variable for something that could just be a
return value from this function.
> filehelper.cpp:44
> +{
> +errno = 0;
> +
should be unnecessary now, with the above suggested change
> filehelper.cpp:134
> +
> +if (errno)
> +reply.setError(errno);
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.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 18012. chinmoyr added a comment. Improved error checking in helper. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=17780=18012 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure added a comment. Not sure I understand your question. If we agree on using return values then there is only one way to do that. E.g. chmod() returns 0 on success, so the code could be like if (chmod(...) == 0) return reply; break; which jumps to the if (errno) code when chmod returns non-zero. Same for all other methods, check the man page for each to avoid making any assumptions, but presumably they all return 0 on success. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added inline comments. INLINE COMMENTS > dfaure wrote in filehelper.cpp:86 > 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. > 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? REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 17780. chinmoyr added a comment. - Add private header - minor fixes CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=17507=17780 BRANCH first REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file_p.h src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> chinmoyr wrote in filehelper.cpp:86
> 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?
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.
> filehelper.cpp:41
> +
> +enum {
> +CHMOD = 1,
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.
> filehelper.cpp:102
> +closedir(dp);
> +}
> +sendFileDescriptor(-1);
missing else or break here, no?
You're sending the valid fd, followed by -1.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added inline comments. INLINE COMMENTS > dfaure wrote in filehelper.cpp:86 > 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? 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? REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 17507. chinmoyr added a comment. - used switch..case CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=17013=17507 REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added a dependent revision: D6829: Add ability to use the new kauth helper in file ioslave. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr edited the summary of this revision. chinmoyr added a dependency: D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added a dependent revision: D6831: Make use of kauth helper in methods of file ioslave. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added a dependent revision: D6830: Make use of kauth helper in copy method of file ioslave. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added a task: T6561: Polkit support in KIO. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr removed a task: T6561: Polkit support in KIO. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr added a task: T6561: Polkit support in KIO. REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure
D6197: Add kauth helper to file ioslave
chinmoyr updated this revision to Diff 17013. chinmoyr retitled this revision from "Add basic KAuth support to file ioslave" to "Add kauth helper to file ioslave". chinmoyr edited the summary of this revision. chinmoyr removed subscribers: davidedmundson, dfaure, eliasp, aacid. chinmoyr added a comment. Removed everything except the helper's code Replaced all the helper method with on method, `exec()`. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=16738=17013 REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure
