[libvirt] [PATCH 0/2] rebase of earlier coverity patch
A while ago, I submitted a patch to plug a hole found by coverity, but there were concerns, I revised it, and the rewrite was never reviewed: https://www.redhat.com/archives/libvir-list/2010-March/msg00443.html I've rebased the series now that 0.8.0 is out.
Based on a warning from coverity. The safe* functions guarantee complete transactions on success, but don't guarantee freedom from failure. * src/util/util.h (saferead, safewrite, safezero): Add ATTRIBUTE_RETURN_CHECK. * src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Ignore some failures. (remoteIOReadBuffer): Adjust error messages on read failure. * daemon/event.c (virEventHandleWakeup): Ignore read failure. --- daemon/event.c | 5 +++-- src/remote/remote_driver.c | 22 ++++++++++++++++------ src/util/util.h | 9 ++++++--- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 2218a3e..6971409 100644 --- a/daemon/event.c +++ b/daemon/event.c @@ -1,8 +1,8 @@ /* * event.c: event loop for monitoring file handles * + * Copyright (C) 2007, 2010 Red Hat, Inc. * Copyright (C) 2007 Daniel P. Berrange - * Copyright (C) 2007 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -35,6 +35,7 @@ #include "event.h" #include "memory.h" #include "util.h" +#include "ignore-value.h" #define EVENT_DEBUG(fmt, ...) DEBUG(fmt, __VA_ARGS__) @@ -630,7 +631,7 @@ static void virEventHandleWakeup(int watch ATTRIBUTE_UNUSED, { char c; virEventLock(); - saferead(fd, &c, sizeof(c)); + ignore_value(saferead(fd, &c, sizeof(c))); virEventUnlock(); } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f514e1d..ebcfcd8 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8877,7 +8877,11 @@ remoteIOReadBuffer(struct private_data *priv, char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + virReportSystemError(errno, "%s", + _("cannot recv data")); + return -1; + } } virReportSystemError(errno, @@ -8886,7 +8890,12 @@ remoteIOReadBuffer(struct private_data *priv, } else { char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + remoteError(VIR_ERR_SYSTEM_ERROR, + _("server closed connection: %s"), + virStrerror(errno, errout, sizeof errout)); + return -1; + } } remoteError(VIR_ERR_SYSTEM_ERROR, @@ -9499,7 +9508,7 @@ remoteIOEventLoop(virConnectPtr conn, sigaddset (&blockedsigs, SIGWINCH); sigaddset (&blockedsigs, SIGCHLD); sigaddset (&blockedsigs, SIGPIPE); - ignore_value (pthread_sigmask(SIG_BLOCK, &blockedsigs, &oldmask)); + ignore_value(pthread_sigmask(SIG_BLOCK, &blockedsigs, &oldmask)); #endif repoll: @@ -9508,14 +9517,15 @@ remoteIOEventLoop(virConnectPtr conn, goto repoll; #ifdef HAVE_PTHREAD_H - ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); + ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); #endif remoteDriverLock(priv); if (fds[1].revents) { DEBUG0("Woken up from poll by other thread"); - saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)); + ignore_value(saferead(priv->wakeupReadFD, &ignore, + sizeof(ignore))); } if (ret < 0) { @@ -9659,7 +9669,7 @@ remoteIO(virConnectPtr conn, priv->waitDispatch = thiscall; /* Force other thread to wakup from poll */ - safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */ diff --git a/src/util/util.h b/src/util/util.h index c256117..4f0b233 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -2,6 +2,7 @@ /* * utils.h: common, generic utility functions * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -31,9 +32,11 @@ # include <sys/select.h> # include <sys/types.h> -int saferead(int fd, void *buf, size_t count); -ssize_t safewrite(int fd, const void *buf, size_t count); -int safezero(int fd, int flags, off_t offset, off_t len); +int saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; +ssize_t safewrite(int fd, const void *buf, size_t count) + ATTRIBUTE_RETURN_CHECK; +int safezero(int fd, int flags, off_t offset, off_t len) + ATTRIBUTE_RETURN_CHECK; enum { VIR_EXEC_NONE = 0, -- 1.6.6.1
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. --- src/remote/remote_driver.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ebcfcd8..b1e8048 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9524,8 +9524,12 @@ remoteIOEventLoop(virConnectPtr conn, if (fds[1].revents) { DEBUG0("Woken up from poll by other thread"); - ignore_value(saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)) + != sizeof(ignore)) { + virReportSystemError(errno ? errno : 0, + "%s", _("read on wakeup fd failed")); + goto error; + } } if (ret < 0) { @@ -9661,6 +9665,7 @@ remoteIO(virConnectPtr conn, /* Stick ourselves on the end of the wait queue */ struct remote_thread_call *tmp = priv->waitDispatch; char ignore = 1; + ssize_t s; while (tmp && tmp->next) tmp = tmp->next; if (tmp) @@ -9668,8 +9673,21 @@ remoteIO(virConnectPtr conn, else priv->waitDispatch = thiscall; - /* Force other thread to wakup from poll */ - ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); + /* Force other thread to wakeup from poll */ + s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + char errout[1024]; + remoteError(VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + virStrerror(errno, errout, sizeof errout)); + VIR_FREE(thiscall); + return -1; + } else if (s != sizeof(ignore)) { + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to wake up polling thread")); + VIR_FREE(thiscall); + return -1; + } DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */ -- 1.6.6.1
Eric Blake wrote:
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. --- src/remote/remote_driver.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ebcfcd8..b1e8048 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9524,8 +9524,12 @@ remoteIOEventLoop(virConnectPtr conn,
if (fds[1].revents) { DEBUG0("Woken up from poll by other thread"); - ignore_value(saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)) + != sizeof(ignore)) { + virReportSystemError(errno ? errno : 0,
This looks fine, but "errno ? errno : 0" is equivalent to just "errno". Which makes me think you'll want to separate the saferead-fails case (where errno is nonzero) from the saferead-returns-non-negative-<=-sizeof-ignore case (in which case virReportSystemError would print "Success" for errno=0).
+ "%s", _("read on wakeup fd failed")); + goto error; + } }
if (ret < 0) { @@ -9661,6 +9665,7 @@ remoteIO(virConnectPtr conn, /* Stick ourselves on the end of the wait queue */ struct remote_thread_call *tmp = priv->waitDispatch; char ignore = 1; + ssize_t s; while (tmp && tmp->next) tmp = tmp->next; if (tmp) @@ -9668,8 +9673,21 @@ remoteIO(virConnectPtr conn, else priv->waitDispatch = thiscall;
- /* Force other thread to wakup from poll */ - ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); + /* Force other thread to wakeup from poll */ + s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + char errout[1024]; + remoteError(VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + virStrerror(errno, errout, sizeof errout)); + VIR_FREE(thiscall); + return -1; + } else if (s != sizeof(ignore)) { + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to wake up polling thread")); + VIR_FREE(thiscall); + return -1; + }
DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */
discussion: https://www.redhat.com/archives/libvir-list/2010-March/msg00443.html * src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. ---
- ignore_value(saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)) + != sizeof(ignore)) { + virReportSystemError(errno ? errno : 0, This looks fine, but "errno ? errno : 0" is equivalent to just "errno". Which makes me think you'll want to separate the saferead-fails case (where errno is nonzero) from the saferead-returns-non-negative-<=-sizeof-ignore case (in which case virReportSystemError would print "Success" for errno=0).
Is this rewrite more what you had in mind? (No change to patch 1/2). src/remote/remote_driver.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ebcfcd8..e3df27b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9523,9 +9523,18 @@ remoteIOEventLoop(virConnectPtr conn, remoteDriverLock(priv); if (fds[1].revents) { + ssize_t s; DEBUG0("Woken up from poll by other thread"); - ignore_value(saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + s = saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)); + if (s < 0) { + virReportSystemError(errno, "%s", + _("read on wakeup fd failed")); + goto error; + } else if (s != sizeof(ignore)) { + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", + _("read on wakeup fd failed")); + goto error; + } } if (ret < 0) { @@ -9661,6 +9670,7 @@ remoteIO(virConnectPtr conn, /* Stick ourselves on the end of the wait queue */ struct remote_thread_call *tmp = priv->waitDispatch; char ignore = 1; + ssize_t s; while (tmp && tmp->next) tmp = tmp->next; if (tmp) @@ -9668,8 +9678,21 @@ remoteIO(virConnectPtr conn, else priv->waitDispatch = thiscall; - /* Force other thread to wakup from poll */ - ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); + /* Force other thread to wakeup from poll */ + s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + char errout[1024]; + remoteError(VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + virStrerror(errno, errout, sizeof errout)); + VIR_FREE(thiscall); + return -1; + } else if (s != sizeof(ignore)) { + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to wake up polling thread")); + VIR_FREE(thiscall); + return -1; + } DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */ -- 1.6.6.1
Eric Blake wrote:
discussion: https://www.redhat.com/archives/libvir-list/2010-March/msg00443.html
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. ---
- ignore_value(saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)) + != sizeof(ignore)) { + virReportSystemError(errno ? errno : 0, This looks fine, but "errno ? errno : 0" is equivalent to just "errno". Which makes me think you'll want to separate the saferead-fails case (where errno is nonzero) from the saferead-returns-non-negative-<=-sizeof-ignore case (in which case virReportSystemError would print "Success" for errno=0).
Is this rewrite more what you had in mind? (No change to patch 1/2).
Thanks. That looks better. No more "Success" diagnostic upon error.
src/remote/remote_driver.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ebcfcd8..e3df27b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9523,9 +9523,18 @@ remoteIOEventLoop(virConnectPtr conn, remoteDriverLock(priv);
if (fds[1].revents) { + ssize_t s; DEBUG0("Woken up from poll by other thread"); - ignore_value(saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + s = saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)); + if (s < 0) { + virReportSystemError(errno, "%s", + _("read on wakeup fd failed")); + goto error; + } else if (s != sizeof(ignore)) { + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", + _("read on wakeup fd failed")); + goto error; + } }
if (ret < 0) { @@ -9661,6 +9670,7 @@ remoteIO(virConnectPtr conn, /* Stick ourselves on the end of the wait queue */ struct remote_thread_call *tmp = priv->waitDispatch; char ignore = 1; + ssize_t s; while (tmp && tmp->next) tmp = tmp->next; if (tmp) @@ -9668,8 +9678,21 @@ remoteIO(virConnectPtr conn, else priv->waitDispatch = thiscall;
- /* Force other thread to wakup from poll */ - ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); + /* Force other thread to wakeup from poll */ + s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + char errout[1024]; + remoteError(VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + virStrerror(errno, errout, sizeof errout)); + VIR_FREE(thiscall); + return -1; + } else if (s != sizeof(ignore)) { + remoteError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to wake up polling thread")); + VIR_FREE(thiscall); + return -1; + }
DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */
On 04/15/2010 11:25 AM, Jim Meyering wrote:
This looks fine, but "errno ? errno : 0" is equivalent to just "errno". Which makes me think you'll want to separate the saferead-fails case (where errno is nonzero) from the saferead-returns-non-negative-<=-sizeof-ignore case (in which case virReportSystemError would print "Success" for errno=0).
Is this rewrite more what you had in mind? (No change to patch 1/2).
Thanks. That looks better. No more "Success" diagnostic upon error.
Thanks for the review; with that approval, I've pushed both patches. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Jim Meyering