| Message ID | 20191111203524.21912-1-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-coroutine-sleep: Silence Coverity warning | expand |
On Mon, 11 Nov 2019 at 20:35, Eric Blake <eblake@redhat.com> 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 > CC: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Eric Blake <eblake@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. The only way to test is to commit it to master and wait for the next run... -- PMM
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.maydell@linaro.org> > Signed-off-by: Eric Blake <eblake@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); > } >
On Tue, 12 Nov 2019 at 10:08, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > 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? Yes, it knows that an assertion means that the condition must be true. thanks -- PMM
Eric Blake <eblake@redhat.com> writes: > 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 > CC: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Eric Blake <eblake@redhat.com> It's a worthwhile documentation of what's going on even if it doesn't shut up coverity. Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
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); + } timer_free(state.ts); }
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 CC: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Eric Blake <eblake@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(+)