Re: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
On 03/24/2011 02:28 PM, Laine Stump wrote:
I noticed you hadn't committed this yet, so I came back to take a
look...
On 03/18/2011 04:46 PM, Eric Blake wrote:
> This simplifies several callers that were repeating checks already
> guaranteed by util.c, and makes other callers more robust to now
> reject directories. remote_driver.c was over-strict - access(,X_OK)
> is not strictly needed to execute a file (although its unusual to see
> a file with X_OK but not R_OK).
In your followup message you wondered whether virFileIsExecutable should
check for readability. I guess if we want to allow people to replace the
commands with shell scripts, then we can do one of two things: 1) check
for readability. Or, 2) just warn people that if they're using a shell
script, they need to make sure it is readable. if they're already
hacking around that much, this is probably a small thing to ask, so I
think until somebody complains it can stay as it is (not checking
readability), since that's the way it already is in all but one case
(and that one is probably the *least* likely to be replaced with a shell
script).
Thanks - that's a workable plan, especially since it's rare to have a
file with execute but not read permissions in the first place.
> @@ -113,7 +113,7 @@ virHookCheck(int no, const char *driver) {
> ret = 0;
> VIR_DEBUG("No hook script %s", path);
> } else {
> - if ((access(path, X_OK) != 0) || (!S_ISREG(sb.st_mode))) {
> + if (!virFileIsExecutable(path)) {
Here you (rightly) removed the examination of sb. That means that the
only result of the call to stat() (just out of view of the diff) used is
the return value. Maybe that could be changed to virFileExists(), or
even just simplify the code and print the same message for "doesn't
exist" and "isn't executable".
The message was only a debug level, so I consolidated things to skip it
altogether.
ACK.
Thanks; pushed with this squashed in:
diff --git i/src/util/hooks.c w/src/util/hooks.c
index 149ff21..99dddc4 100644
--- i/src/util/hooks.c
+++ w/src/util/hooks.c
@@ -94,13 +94,12 @@ static int virHooksFound = -1;
static int
virHookCheck(int no, const char *driver) {
char *path;
- struct stat sb;
int ret;
if (driver == NULL) {
virHookReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid hook name for #%d"), no);
- return(-1);
+ return -1;
}
ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, driver);
@@ -108,24 +107,19 @@ virHookCheck(int no, const char *driver) {
virHookReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to build path for %s hook"),
driver);
- return(-1);
+ return -1;
}
- if (stat(path, &sb) < 0) {
+ if (!virFileIsExecutable(path)) {
ret = 0;
- VIR_DEBUG("No hook script %s", path);
+ VIR_WARN("Missing or non-executable hook script %s", path);
} else {
- if (!virFileIsExecutable(path)) {
- ret = 0;
- VIR_WARN("Non executable hook script %s", path);
- } else {
- ret = 1;
- VIR_DEBUG("Found hook script %s", path);
- }
+ ret = 1;
+ VIR_DEBUG("Found hook script %s", path);
}
VIR_FREE(path);
- return(ret);
+ return ret;
}
/*
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)