| Message ID | 20220111121059.3345034-1-daniellalee111@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix null pointer dereference in util/fdmon-epoll.c | expand |
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 >
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 {
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(-)