Re: [Qemu-devel] [Qemu-block] [PATCH 5/6] curl: Handle success in multi_check_completion

[Max Reitz]

On 09.09.19 22:30, John Snow wrote:
> 
> 
> On 8/27/19 12:34 PM, 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 read 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 bc70f39fcb..5e0cca601d 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -231,7 +231,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);
>>  
>> @@ -247,32 +246,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)) {
> 
> This changes from a conditional,

It doesn’t really change.  This code here is the read_cb.  We may have
incomplete data still.

>> -            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;
>> @@ -353,13 +326,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
>> @@ -371,20 +345,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);
> 
> To an assertion. This makes me feel better (What happens if that was
> ever false in the old code, we just drop the callback?) but is it
> definitely always gonna be true?

No, in the old code, the read_cb may have been called multiple times per
request.  Of course, it would only really complete the last time, and
then we’d invoke the callback.

But here we just look at all requests that are done (CURLMSG_DONE from
curl_multi_info_read()).  So we can now assert that we’ve actually read
all data.

You could argue that it’s up to the library to behave correctly for this
assertion not to blow up, which may not be to everyone’s liking.

> Well, you are asserting it is, so I will believe you.
> 
>> +
>> +                    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);
>> +                    }
>>                  }
>> +
>> +                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);
>>
> 
> Only other thing that's not obvious to someone who hasn't programmed
> curl before: this action is moving from curl_read_cb to check_completion.

What you should ask here is why we ever did handle it in read_cb rather
than here.  That’s simply because the code in read_cb is older than
curl_multi_check_completion().  (f785a5ae36c added the error handling;
including the “ACBs for successful messages get completed in
curl_read_cb” comment.)

> check_completion is only called in curl_multi_read and not
> curl_multi_do, but curl_read_cb is arguably called somewhere down the
> stack from curl_multi_do_locked.
> 
> I assume because this is curl_read_cb that there was no way it was
> getting invoked from curl_multi_do, and therefore we're not missing
> something on the return trip now.

Two things:

First, we do all error handling in check_completion, so we already rely
on it being invoked at some point (or failed requests would stall).

Second, I cannot give you a guarantee.  It isn’t like I have cURL in my
blood.  But from what I gather, it doesn’t do anything in the
background, at least if the user (that is us) handles all the timer and
FD stuff.  So it should indeed only invoke the read_cb from whenever we
tell it that new data is available on some FD (or maybe when some timer
fires).  We do call curl_multi_check_completion() whenever new data is
available for reading (or when a timer fires), so it looks OK to me.
We just don’t call it whenever cURL can write to the socket.  But it
can’t read anything from it then, so it should be OK.


Now that’s an awful lot of “should” and “looks OK”.  We can do one of
two things to improve the situation, I think: curl_multi_socket_action()
actually takes an argument that tells it whether the socket is available
for reading or writing (currently we always pass 0 to it, which means
“figure it out yourself”).  This could ensure that it doesn’t read data
when we think the socket is only available for writing and thus won’t
call check_completion.

But there’s a problem: cURL doesn’t only accept IN and OUT there, but
also ERR.  When do we pass that?  Both in _read and _do?  But if it gets
ERR, it will probably return an error, so we should then call
check_completion.  So should we pass IN | ERR from _read and just OUT
from _do?

It gets a bit icky and I don’t want to add a bug here.

So there’s of course something else we could do which is to simply
invoke check_completion in _do for good measure.  (And then _do and
_read would be the same, so we could even save some LoC.)

What do you think?

Max

signature.asc
Description: OpenPGP digital signature