On Fri, 2019-09-13 at 13:20 +0200, Max Reitz wrote:
> On 10.09.19 18:13, Maxim Levitsky wrote:
> > On Tue, 2019-09-10 at 14:41 +0200, Max Reitz wrote:
> > > Background: As of cURL 7.59.0, it verifies that several functions are
> > > not called from within a callback. Among these functions is
> > > curl_multi_add_handle().
> > >
> > > curl_read_cb() is a callback from cURL and not a coroutine. Waking up
> > > acb->co will lead to entering it then and there, which means the current
> > > request will settle and the caller (if it runs in the same coroutine)
> > > may then issue the next request. In such a case, we will enter
> > > curl_setup_preadv() effectively from within curl_read_cb().
> > >
> > > Calling curl_multi_add_handle() will then fail and the new request will
> > > not be processed.
> > >
> > > Fix this by not letting curl_read_cb() wake up acb->co. Instead, leave
> > > the whole business of settling the AIOCB objects to
> > > curl_multi_check_completion() (which is called from our timer callback
> > > and our FD handler, so not from any cURL callbacks).
> > >
> > > Reported-by: Natalie Gavrielov <ngavr...@redhat.com>
> > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
> > > Cc: qemu-sta...@nongnu.org
> > > Signed-off-by: Max Reitz <mre...@redhat.com>
> > > ---
> > > block/curl.c | 69 ++++++++++++++++++++++------------------------------
> > > 1 file changed, 29 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/block/curl.c b/block/curl.c
> > > index fd70f1ebc4..c343c7ed3d 100644
> > > --- a/block/curl.c
> > > +++ b/block/curl.c
> > > @@ -229,7 +229,6 @@ static size_t curl_read_cb(void *ptr, size_t size,
> > > size_t nmemb, void *opaque)
> > > {
> > > CURLState *s = ((CURLState*)opaque);
> > > size_t realsize = size * nmemb;
> > > - int i;
> > >
> > > trace_curl_read_cb(realsize);
> > >
> > > @@ -245,32 +244,6 @@ static size_t curl_read_cb(void *ptr, size_t size,
> > > size_t nmemb, void *opaque)
> > > memcpy(s->orig_buf + s->buf_off, ptr, realsize);
> > > s->buf_off += realsize;
> > >
> > > - for(i=0; i<CURL_NUM_ACB; i++) {
> > > - CURLAIOCB *acb = s->acb[i];
> > > -
> > > - if (!acb)
> > > - continue;
> > > -
> > > - if ((s->buf_off >= acb->end)) {
> > > - size_t request_length = acb->bytes;
> > > -
> > > - qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
> > > - acb->end - acb->start);
> > > -
> > > - if (acb->end - acb->start < request_length) {
> > > - size_t offset = acb->end - acb->start;
> > > - qemu_iovec_memset(acb->qiov, offset, 0,
> > > - request_length - offset);
> > > - }
> > > -
> > > - acb->ret = 0;
> > > - s->acb[i] = NULL;
> > > - qemu_mutex_unlock(&s->s->mutex);
> > > - aio_co_wake(acb->co);
> > > - qemu_mutex_lock(&s->s->mutex);
> > > - }
> > > - }
> > > -
> > > read_end:
> > > /* curl will error out if we do not return this value */
> > > return size * nmemb;
> > > @@ -351,13 +324,14 @@ static void
> > > curl_multi_check_completion(BDRVCURLState *s)
> > > break;
> > >
> > > if (msg->msg == CURLMSG_DONE) {
> > > + int i;
> > > CURLState *state = NULL;
> > > + bool error = msg->data.result != CURLE_OK;
> > > +
> > > curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE,
> > > (char **)&state);
> > >
> > > - /* ACBs for successful messages get completed in
> > > curl_read_cb */
> > > - if (msg->data.result != CURLE_OK) {
> > > - int i;
> > > + if (error) {
> > > static int errcount = 100;
> > >
> > > /* Don't lose the original error message from curl, since
> > > @@ -369,20 +343,35 @@ static void
> > > curl_multi_check_completion(BDRVCURLState *s)
> > > error_report("curl: further errors suppressed");
> > > }
> > > }
> > > + }
> > >
> > > - for (i = 0; i < CURL_NUM_ACB; i++) {
> > > - CURLAIOCB *acb = state->acb[i];
> > > + for (i = 0; i < CURL_NUM_ACB; i++) {
> > > + CURLAIOCB *acb = state->acb[i];
> > >
> > > - if (acb == NULL) {
> > > - continue;
> > > - }
> > > + if (acb == NULL) {
> > > + continue;
> > > + }
> > > +
> > > + if (!error) {
> > > + /* Assert that we have read all data */
> > > + assert(state->buf_off >= acb->end);
> > > +
> > > + qemu_iovec_from_buf(acb->qiov, 0,
> > > + state->orig_buf + acb->start,
> > > + acb->end - acb->start);
> > >
> > > - acb->ret = -EIO;
> > > - state->acb[i] = NULL;
> > > - qemu_mutex_unlock(&s->mutex);
> > > - aio_co_wake(acb->co);
> > > - qemu_mutex_lock(&s->mutex);
> > > + if (acb->end - acb->start < acb->bytes) {
> > > + size_t offset = acb->end - acb->start;
> > > + qemu_iovec_memset(acb->qiov, offset, 0,
> > > + acb->bytes - offset);
> > > + }
> >
> > Original code was memsetting the tail of the buffer before waking up the
> > coroutine.
> > Is this change intended?
> >
> > aio_co_wake doesn't enter the co-routine if already in coroutine, but
> > I think that this is an aio fd handler with isn't run in co-routine itself,
> > so the callback could run with not yet ready data.
>
> (Sorry, I don’t know why I forgot to reply.)
>
> I don’t see how anything changes. aio_co_wake() is still called after
> the qemu_iovec_memset().
Oops, in this specific areas of the patch, I totally missed the diff markers.
So,
Reviewed-by: Maxim Levitsky <mlevi...@redhat.com>
Best regards,
Maxim Levitsky
