[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Fix null pointer dereference in util/fdmon-epoll.c
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH] Fix null pointer dereference in util/fdmon-epoll.c |
Date: |
Thu, 13 Jan 2022 14:16:12 +0000 |
On Tue, Jan 11, 2022 at 08:10:59PM +0800, Daniella Lee wrote:
> Orginal qemu commit hash: de3f5223fa4cf8bfc5e3fe1fd495ddf468edcdf7
> In util/fdmon-epoll.c, function fdmon_epoll_update, variable "old_node"
> maybe NULL with the condition, while it is directly used in the statement and
> may lead to null pointer dereferencen problem.
> Variable "r" in the condition is the return value of epoll_ctl function,
> and will return -1 when failed.
> Therefore, the patch added a check and initialized the variable "r".
>
>
> Signed-off-by: Daniella Lee <daniellalee111@gmail.com>
> ---
> util/fdmon-epoll.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Hi Daniella,
Thanks for the patch! How is the new_node == NULL && old_node == NULL
case reached?
The caller is util/aio-posix.c:aio_set_fd_handler():
AioHandler *node;
AioHandler *new_node = NULL;
...
node = find_aio_handler(ctx, fd);
/* Are we deleting the fd handler? */
if (!io_read && !io_write && !io_poll) {
if (node == NULL) {
qemu_lockcnt_unlock(&ctx->list_lock);
return; /* old_node == NULL && new_node == NULL */
}
... /* old_node != NULL && new_node == NULL */
} else {
...
new_node = g_new0(AioHandler, 1);
...
}
/* (old_node != NULL && new_node == NULL) || (new_node != NULL) */
...
ctx->fdmon_ops->update(ctx, node, new_node);
aio_set_fd_handler() returns early instead of calling ->update() when
old_node == NULL && new_node == NULL. It looks like the NULL pointer
dereference cannot happen and semantically it doesn't make sense to call
->update(ctx, NULL, NULL) since there is nothing to update so it's
unlikely to be called this way in the future.
Have I missed something?
Thanks,
Stefan
> diff --git a/util/fdmon-epoll.c b/util/fdmon-epoll.c
> index e11a8a022e..3c8b0de694 100644
> --- a/util/fdmon-epoll.c
> +++ b/util/fdmon-epoll.c
> @@ -38,10 +38,12 @@ static void fdmon_epoll_update(AioContext *ctx,
> .data.ptr = new_node,
> .events = new_node ? epoll_events_from_pfd(new_node->pfd.events) : 0,
> };
> - int r;
> + int r = -1;
>
> if (!new_node) {
> - r = epoll_ctl(ctx->epollfd, EPOLL_CTL_DEL, old_node->pfd.fd, &event);
> + if (old_node) {
> + r = epoll_ctl(ctx->epollfd, EPOLL_CTL_DEL, old_node->pfd.fd,
> &event);
> + }
> } else if (!old_node) {
> r = epoll_ctl(ctx->epollfd, EPOLL_CTL_ADD, new_node->pfd.fd, &event);
> } else {
> --
> 2.17.1
>
signature.asc
Description: PGP signature