Re: [PATCH] qemu-coroutine-sleep: Silence Coverity warning

[Vladimir Sementsov-Ogievskiy]

11.11.2019 23:35, Eric Blake wrote:
> Coverity warns that we store the address of a stack variable through a
> pointer passed in by the caller, which would let the caller trivially
> trigger use-after-free if that stored value is still present when we
> finish execution.  However, the way coroutines work is that after our
> call to qemu_coroutine_yield(), control is temporarily continued in
> the caller prior to our function concluding, and in order to resume
> our coroutine, the caller must poll until the variable has been set to
> NULL.  Thus, we can add an assert that we do not leak stack storage to
> the caller on function exit.
> 
> Fixes: Coverity CID 1406474

Hmm, I doubt that it will fix it.. Do Coverity pay attention to assertions?

> CC: Peter Maydell <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> I don't know if this actually shuts Coverity up; Peter, since you
> reported the Coverity issue, are you in a better position to test if
> this makes a difference?  At any rate, the tests still pass after
> this is in place.
> 
> I'm not sure if the compiler wants us to insert a 'volatile' in any
> of our uses of QemuCoSleepState.user_state_pointer.
> 
>   util/qemu-coroutine-sleep.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
> index ae91b92b6e78..769a76e57df0 100644
> --- a/util/qemu-coroutine-sleep.c
> +++ b/util/qemu-coroutine-sleep.c
> @@ -68,5 +68,12 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType 
> type, int64_t ns,
>       }
>       timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
>       qemu_coroutine_yield();
> +    if (sleep_state) {
> +        /*
> +         * Note that *sleep_state is cleared during qemu_co_sleep_wake
> +         * before resuming this coroutine.
> +         */
> +        assert(*sleep_state == NULL);
> +    }

Hmm.. sleep_state is not owned by this code, and this is possible that user of 
the API may want
to reuse the variable (for another call to qemu_co_sleep_ns_wakeable) 
immediately after calling
qemu_co_sleep_wake (which don't call qemu_coroutine_enter but aio_co_wake)..

Seems, current usage in nbd code is not the case, so we may add this assertion, 
but I'd add a comment
to qemu_co_sleep_ns_wakeable, that sleep_state must not be reused until 
qemu_co_sleep_ns_wakeable
finish.. It seems obvious, but actually it isn't, keeping in mind that 
qemu_co_sleep_wake() call
may be interpreted as a point where sleep_state is freed..


>       timer_free(state.ts);
>   }
> 


-- 
Best regards,
Vladimir