[libvirt] [PATCH] event: don't queue NULL event on OOM

Ever since commit 61ac8ce, Coverity complained about remoteNetworkBuildEventLifecycle not checking for NULL failure to build an event, compared to other calls in the code base. But the problem is latent from copy and paste; all 17 of our remote*BuildEvent* functions in remote_driver.c have the same issue - if an OOM causes an event to not be built, we happily pass NULL to remoteEventQueue(), but that function has marked event as a nonnull parameter. We were getting lucky (the event queue's first use of the event happened to be a call to virIsObjectClass(), which acts gracefully on NULL, so there was no way to crash); but this is a latent bug waiting to bite us due to the disregard for the nonnull attribute, as well as a waste of resources in the event queue. Better is to just refuse to queue NULL. The discard is silent, since the problem only happens on OOM, and since events are already best effort - if we fail to get an event, it's not like we have any memory left to report the issue, nor any idea of who would benefit from knowing we couldn't create or queue the event. * src/remote/remote_driver.c (remoteEventQueue): Ignore NULL event. Signed-off-by: Eric Blake <eblake@redhat.com> --- I don't know if this will actually shut up Coverity, or if we have to modify all 17 calls to remoteEventQueue to do the NULL check there. I'm hoping this simpler solution does the trick. src/remote/remote_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 64d9d92..da9c1c9 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6801,7 +6801,8 @@ done: static void remoteEventQueue(struct private_data *priv, virObjectEventPtr event) { - virObjectEventStateQueue(priv->eventState, event); + if (event) + virObjectEventStateQueue(priv->eventState, event); } /* get_nonnull_domain and get_nonnull_network turn an on-wire -- 1.8.4.2

On 01/09/2014 10:22 PM, Eric Blake wrote:
Ever since commit 61ac8ce, Coverity complained about remoteNetworkBuildEventLifecycle not checking for NULL failure to build an event, compared to other calls in the code base. But the problem is latent from copy and paste; all 17 of our remote*BuildEvent* functions in remote_driver.c have the same issue - if an OOM causes an event to not be built, we happily pass NULL to remoteEventQueue(), but that function has marked event as a nonnull parameter. We were getting lucky (the event queue's first use of the event happened to be a call to virIsObjectClass(), which acts gracefully on NULL, so there was no way to crash); but this is a latent bug waiting to bite us due to the disregard for the nonnull attribute, as well as a waste of resources in the event queue. Better is to just refuse to queue NULL. The discard is silent, since the problem only happens on OOM, and since events are already best effort - if we fail to get an event, it's not like we have any memory left to report the issue, nor any idea of who would benefit from knowing we couldn't create or queue the event.
* src/remote/remote_driver.c (remoteEventQueue): Ignore NULL event.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
I don't know if this will actually shut up Coverity, or if we have to modify all 17 calls to remoteEventQueue to do the NULL check there. I'm hoping this simpler solution does the trick.
src/remote/remote_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK

On 01/09/2014 08:25 PM, John Ferlan wrote:
On 01/09/2014 10:22 PM, Eric Blake wrote:
Ever since commit 61ac8ce, Coverity complained about remoteNetworkBuildEventLifecycle not checking for NULL failure to build an event, compared to other calls in the code base.
I don't know if this will actually shut up Coverity, or if we have to modify all 17 calls to remoteEventQueue to do the NULL check there. I'm hoping this simpler solution does the trick.
src/remote/remote_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK
You can't have finished a Coverity run that fast! But here's hoping it works (and re-reading the original Coverity report, I think it will):
(15) Event dereference: Dereferencing a pointer that might be null "event" when calling "remoteDomainEventQueue(struct private_data *, virObjectEventPtr)". [details] Also see events: [returned_null][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][example_assign][example_checked][var_assigned]
so Coverity is already smart enough to trace the flow from *BuildEvent* to remoteDomainEventQueue() to the non-null parameter of virObjectEventStateQueue(). So I pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
John Ferlan