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 <peter.mayd...@linaro.org>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>
> 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
