On 03/09/2016 12:38 PM, Cole Robinson wrote:
Break these checks out into their own function, and clearly document each one. This shouldn't change behavior --- src/util/virfile.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index f45e18f..cea2674 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2314,6 +2314,32 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, }
+/* virFileRemoveNeedsSetuid: + * @uid: file uid to check + * @gid: file gid to check + * + * Return true if we should use setuid/setgid before deleting a file + * owned by the passed uid/gid pair. Needed for NFS with root-squash + */ +static bool +virFileRemoveNeedsSetuid(uid_t uid, gid_t gid) +{ + /* If running unprivileged, setuid isn't going to work */ + if (geteuid() != 0) + return false; + + /* uid/gid weren'd specified */
weren't ACK - with the nit fixed... The rest is me typing out my thoughts... John
+ if ((uid == (uid_t) -1) && (gid == (gid_t) -1)) + return false;
This "should" only happen as failure path of virStorageBackendCreateExecCommand when uid/gid not provided in the XML. The virStorageBackendCreateRaw failure path expects creation to have occurred where virFileOpenAs would have set the owner/mode due to the operation_flags setting.
+ + /* already running as proper uid/gid */ + if (uid == geteuid() && gid == getegid()) + return false; +
If the XML provided uid/gid and it's root - that's what we want
+ return true; +} + + /* virFileRemove: * @path: file to unlink or directory to remove * @uid: uid that was used to create the file (not required) @@ -2335,12 +2361,7 @@ virFileRemove(const char *path, gid_t *groups; int ngroups;
- /* If not running as root or if a non explicit uid/gid was being used for - * the file/volume or the explicit uid/gid matches, then use unlink directly - */ - if ((geteuid() != 0) || - ((uid == (uid_t) -1) && (gid == (gid_t) -1)) || - (uid == geteuid() && gid == getegid())) { + if (!virFileRemoveNeedsSetuid(uid, gid)) { if (virFileIsDir(path)) return rmdir(path); else