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(-)

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

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
>