Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update

[Paolo Bonzini]

On 07/11/2016 15:58, Stefan Hajnoczi wrote:
> On Mon, Nov 07, 2016 at 10:33:33AM +0100, Paolo Bonzini wrote:
>> aio_epoll_update dereferences parameter "node", but it could have been NULL
>> if deleting an fd handler that was not registered in the first place.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  aio-posix.c | 33 ++++++++++++++++++---------------
>>  1 file changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/aio-posix.c b/aio-posix.c
>> index 4ef34dd..ec908f7 100644
>> --- a/aio-posix.c
>> +++ b/aio-posix.c
>> @@ -217,21 +217,24 @@ void aio_set_fd_handler(AioContext *ctx,
>>  
>>      /* Are we deleting the fd handler? */
>>      if (!io_read && !io_write) {
>> -        if (node) {
>> -            g_source_remove_poll(&ctx->source, &node->pfd);
>> -
>> -            /* If the lock is held, just mark the node as deleted */
>> -            if (ctx->walking_handlers) {
>> -                node->deleted = 1;
>> -                node->pfd.revents = 0;
>> -            } else {
>> -                /* Otherwise, delete it for real.  We can't just mark it as
>> -                 * deleted because deleted nodes are only cleaned up after
>> -                 * releasing the walking_handlers lock.
>> -                 */
>> -                QLIST_REMOVE(node, node);
>> -                deleted = true;
>> -            }
>> +        if (node == NULL) {
>> +            return;
>> +        }
>> +
>> +        node->pfd.events = 0;
> 
> ^--- is this left over from debugging...

No, it's left over from solving conflicts.  This is an old patch that
got lost.  Will send v2, thanks for the review!

Paolo

>> +        g_source_remove_poll(&ctx->source, &node->pfd);
>> +
>> +        /* If the lock is held, just mark the node as deleted */
>> +        if (ctx->walking_handlers) {
>> +            node->deleted = 1;
>> +            node->pfd.revents = 0;
> 
> ...the original code clears revents here?
>