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.
> }
> +
> + 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
