[libvirt] [PATCH] util: Don't overflow on errno in virFileAccessibleAs
If we need to virFork() to check assess() under different UID+GID we need to translate returned status via WEXITSTATUS(). Otherwise, we may return values greater than 255 which is obviously wrong. --- src/util/util.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 548ed1c..15e6cfa 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -724,8 +724,13 @@ virFileAccessibleAs(const char *path, int mode, return -1; } + if (!WIFEXITED(status)) { + errno = EINTR; + return -1; + } + if (status) { - errno = status; + errno = WEXITSTATUS(status); return -1; } -- 1.7.8.5
On 03/08/2012 03:37 AM, Michal Privoznik wrote:
If we need to virFork() to check assess() under different UID+GID we need to translate returned status via WEXITSTATUS(). Otherwise, we may return values greater than 255 which is obviously wrong. --- src/util/util.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 548ed1c..15e6cfa 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -724,8 +724,13 @@ virFileAccessibleAs(const char *path, int mode, return -1; }
+ if (!WIFEXITED(status)) { + errno = EINTR; + return -1; + }
ACK; this matches what we do in virFileOpenForked. However, I still see two lingering issues that might be worth revisiting: 1. I wonder if virWaitPid() would be easier to use if it only returned success on WIFEXITED, and set *status to WEXITSTAUS(), while returning -1 on any child dying due to a signal. I'd have to audit the users of virWaitPid to see if they can all be simplified by this change, or if there really is a user that needs to know if a child exited due to a signal. 2. This still shares the latent bug in virFileOpenForked that errno is not always guaranteed to be less than 255; on GNU Hurd, this code is broken - but libvirt doesn't compile on Hurd. A true fix would be to enumerate specific errno values to specific exit codes, and map all others to a catch-all; see how daemon/libvirtd.c has virDaemonErr for this purpose. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08.03.2012 14:08, Eric Blake wrote:
On 03/08/2012 03:37 AM, Michal Privoznik wrote:
If we need to virFork() to check assess() under different UID+GID we need to translate returned status via WEXITSTATUS(). Otherwise, we may return values greater than 255 which is obviously wrong. --- src/util/util.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 548ed1c..15e6cfa 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -724,8 +724,13 @@ virFileAccessibleAs(const char *path, int mode, return -1; }
+ if (!WIFEXITED(status)) { + errno = EINTR; + return -1; + }
ACK; this matches what we do in virFileOpenForked.
Thanks pushed.
However, I still see two lingering issues that might be worth revisiting:
1. I wonder if virWaitPid() would be easier to use if it only returned success on WIFEXITED, and set *status to WEXITSTAUS(), while returning -1 on any child dying due to a signal. I'd have to audit the users of virWaitPid to see if they can all be simplified by this change, or if there really is a user that needs to know if a child exited due to a signal.
yes, i was wondering about this too when writing the patch. However I took the quicker way. Let me see if i can produce cleanup patch as you've described it.
2. This still shares the latent bug in virFileOpenForked that errno is not always guaranteed to be less than 255; on GNU Hurd, this code is broken - but libvirt doesn't compile on Hurd. A true fix would be to enumerate specific errno values to specific exit codes, and map all others to a catch-all; see how daemon/libvirtd.c has virDaemonErr for this purpose.
Yeah, since we don't compile on Hurd anyway, I wouldn't take much care here. I am not saying we should make it intentionally harder for a developer trying to make libvirt compile there, but why unnecessarily bound ourselves? Michal
participants (2)
-
Eric Blake -
Michal Privoznik