Re: [PATCH] Fix null pointer dereference in util/fdmon-epoll.c

[Stefan Hajnoczi]

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 <daniellalee...@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