Re: [Qemu-stable] [Qemu-block] [PATCH v2 6/7] curl: Handle success in multi_check_completion

[Max Reitz]

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 <address@hidden>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1740193
>> Cc: address@hidden
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  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().

Max

>>                  }
>> +
>> +                acb->ret = error ? -EIO : 0;
>> +                state->acb[i] = NULL;
>> +                qemu_mutex_unlock(&s->mutex);
>> +                aio_co_wake(acb->co);
>> +                qemu_mutex_lock(&s->mutex);
>>              }
>>  
>>              curl_clean_state(state);
> 
> 
> Best regards,
>       Maxim Levitsky
>

signature.asc
Description: OpenPGP digital signature