[libvirt] [PATCH] Use virFileResolveLink instead of readlink in AppArmor
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/security/security_apparmor.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index f288645..0857d58 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -258,22 +258,23 @@ get_profile_name(virConnectPtr conn, virDomainObjPtr vm) static int use_apparmor(void) { - char libvirt_daemon[PATH_MAX]; int rc = -1; - ssize_t len = 0; + char *libvirt_daemon = NULL; - if ((len = readlink("/proc/self/exe", libvirt_daemon, - PATH_MAX - 1)) < 0) { + if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virSecurityReportError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("could not find libvirtd")); - return rc; + return -1; } - libvirt_daemon[len] = '\0'; if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) - return rc; + goto cleanup; - return profile_status(libvirt_daemon, 1); + rc = profile_status(libvirt_daemon, 1); + +cleanup: + VIR_FREE(libvirt_daemon); + return rc; } /* Called on libvirtd startup to see if AppArmor is available */ -- 1.6.6
Was this patch tested? I initially tried to use virFileResolveLink() but could not. This was discussed in "[libvirt] [PATCH 0/4] AppArmor updates" from 11/12/2009: "Implements all changes requested by DV except for getting rid of readlink(). I can't use virFileResolveLink() because it lstat()s the file and uses st.st_size to create a buffer. Unfortunately, running lstat() on /proc/self/exe results in st.st_size to be 0." Unless virFileResolveLink() was changed, this patch will break AppArmor. Jamie -- Jamie Strandboge | http://www.canonical.com
On 01/21/2010 11:49 AM, Jamie Strandboge wrote:
Was this patch tested? I initially tried to use virFileResolveLink() but could not. This was discussed in "[libvirt] [PATCH 0/4] AppArmor updates" from 11/12/2009:
"Implements all changes requested by DV except for getting rid of readlink(). I can't use virFileResolveLink() because it lstat()s the file and uses st.st_size to create a buffer. Unfortunately, running lstat() on /proc/self/exe results in st.st_size to be 0."
Unless virFileResolveLink() was changed, this patch will break AppArmor.
Jamie
Ah, I should have mentioned that. I only compile tested it. However, virFileResolveLink() was indeed changed to use gnulib's areadlink() function instead of the previous lstat()/readlink(), so it may work now. Do you have a test environment to test it out quickly? -- Chris Lalancette
2010/1/21 Jamie Strandboge <jamie@canonical.com>:
Was this patch tested? I initially tried to use virFileResolveLink() but could not. This was discussed in "[libvirt] [PATCH 0/4] AppArmor updates" from 11/12/2009:
"Implements all changes requested by DV except for getting rid of readlink(). I can't use virFileResolveLink() because it lstat()s the file and uses st.st_size to create a buffer. Unfortunately, running lstat() on /proc/self/exe results in st.st_size to be 0."
Unless virFileResolveLink() was changed, this patch will break AppArmor.
Jamie
-- Jamie Strandboge | http://www.canonical.com
I think Jim Meyering fixed virFileResolveLink, see: http://libvirt.org/git/?p=libvirt.git;a=commit;h=5baa463541d9081d43e86237e88... Matthias
On Thu, 2010-01-21 at 18:03 +0100, Matthias Bolte wrote:
I think Jim Meyering fixed virFileResolveLink, see:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=5baa463541d9081d43e86237e88...
I missed this commit. It seems Jim was specifically addressing the issue I had in his commit. As such, I stand corrected. Feel free to apply the patch and I'll test. If I have problems, I'll report back. Jamie -- Jamie Strandboge | http://www.canonical.com
On 01/21/2010 12:14 PM, Jamie Strandboge wrote:
On Thu, 2010-01-21 at 18:03 +0100, Matthias Bolte wrote:
I think Jim Meyering fixed virFileResolveLink, see:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=5baa463541d9081d43e86237e88...
I missed this commit. It seems Jim was specifically addressing the issue I had in his commit. As such, I stand corrected. Feel free to apply the patch and I'll test. If I have problems, I'll report back.
Thanks, I've pushed this now. -- Chris Lalancette
On Thu, 2010-01-21 at 11:33 -0500, Chris Lalancette wrote:
--- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -258,22 +258,23 @@ get_profile_name(virConnectPtr conn, virDomainObjPtr vm) static int use_apparmor(void) { - char libvirt_daemon[PATH_MAX]; int rc = -1; - ssize_t len = 0; + char *libvirt_daemon = NULL;
- if ((len = readlink("/proc/self/exe", libvirt_daemon, - PATH_MAX - 1)) < 0) { + if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virSecurityReportError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("could not find libvirtd")); - return rc; + return -1;
I've yet to test areadlink() from gnulib, but in the meantime, I'd prefer if this were 'return rc' since rc is already '-1'. Jamie -- Jamie Strandboge | http://www.canonical.com
On 01/21/2010 12:24 PM, Jamie Strandboge wrote:
On Thu, 2010-01-21 at 11:33 -0500, Chris Lalancette wrote:
--- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -258,22 +258,23 @@ get_profile_name(virConnectPtr conn, virDomainObjPtr vm) static int use_apparmor(void) { - char libvirt_daemon[PATH_MAX]; int rc = -1; - ssize_t len = 0; + char *libvirt_daemon = NULL;
- if ((len = readlink("/proc/self/exe", libvirt_daemon, - PATH_MAX - 1)) < 0) { + if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virSecurityReportError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("could not find libvirtd")); - return rc; + return -1;
I've yet to test areadlink() from gnulib, but in the meantime, I'd prefer if this were 'return rc' since rc is already '-1'.
Yeah, it doesn't really matter to me either way. I'll fold your suggested change in when I apply. Thanks for the review. -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Jamie Strandboge -
Matthias Bolte