Skip to content

CURL_READFUNC_ABORT is lost if returned in a middle of transfer #4813

Closed
@MrdUkk

Description

@MrdUkk

I did this

I'm uploading multipart form data (large stream) to server.
    I'm doing it by using curl_easy_perform() in thread1 with specified CURLOPT_READFUNCTION
   CURLOPT_READDATA, CURLOPT_WRITEDATA, CURLOPT_NOPROGRESS =0.

in a middle of transfer want to cancel transfer. so i return in my callback function to CURL specified
CURL_READFUNC_ABORT value.

I expected the following

expect to see immediately aborted transfer but libcurl REPEATEDLY call again and again my callback function and doesn't stop!

curl/libcurl version

7.68.0

operating system

VS2015 Windows 8.1.

after debugging i find out some strange behaviour in mime.c function
readback_part() calling sz = read_part_content(part, buffer, bufsize);
we are returning in sz CURL_READFUNC_ABORT value , stepping next switch

  switch(sz) {
  case 0:
    mimesetstate(&part->state, MIMESTATE_END, NULL);
    /* Try sparing open file descriptors. */
    if(part->kind == MIMEKIND_FILE && part->fp) {
      fclose(part->fp);
      part->fp = NULL;
    }
    /* FALLTHROUGH */
  case CURL_READFUNC_ABORT:
  case CURL_READFUNC_PAUSE:
  case READ_ERROR:
    return cursize? cursize: sz;
  }

here i see random non zero 'cursize' values (is it bytes transfered already before we abort?)
and cursize is not zero it will don't return (losing) proper CURL_READFUNC_ABORT to upper levels of callstack.
it's sounds something simular to old 2011 bug . (see git repo commit 840eff4).
transfer isn't stopped and hung in curl_easy_perform()

Activity

bagder

bagder commented on Jan 14, 2020

@bagder
Member

This sounds annoying. Can you create a simple source code that reproduces this problem for us to use to debug this?

jay

jay commented on Jan 14, 2020

@jay
Member
bagder

bagder commented on Jan 14, 2020

@bagder
Member

The libcurl test lib643.c uses this callback. I'll see if I can just adjust that and create a test case for this scenario...

bagder

bagder commented on Jan 14, 2020

@bagder
Member

Even better: test 644 actually verifies formpost with CURL_READFUNC_ABORT. The question is then what we need to adjust in there to reproduce @MrdUkk's problem. Any suggestion?

That test returns abort immediately, like this:

return CURL_READFUNC_ABORT;

jay

jay commented on Jan 14, 2020

@jay
Member

His analysis may be correct though and I don't know why it would not return abort either. That pattern is in the code several times, Patrick put it there, let's see what he has to say.

bagder

bagder commented on Jan 14, 2020

@bagder
Member

I figured it out. Test 644 simply isn't a good test case. The callback keeps getting called, exactly as reported here, but the code doesn't detect or fail the test because of it.

To improve test 644 (and thus reproduce this bug):

--- a/tests/libtest/lib643.c
+++ b/tests/libtest/lib643.c
@@ -39,14 +39,19 @@ struct WriteThis {
 };
 
 static size_t read_callback(char *ptr, size_t size, size_t nmemb, void *userp)
 {
 #ifdef LIB644
+  static int count = 0;
   (void)ptr;
   (void)size;
   (void)nmemb;
   (void)userp;
+  if(++count > 1) {
+    printf("Wrongly called >1 times\n");
+    exit(1); /* trigger major failure */
+  }
   return CURL_READFUNC_ABORT;
 #else
 
   struct WriteThis *pooh = (struct WriteThis *)userp;
   int eof = !*pooh->readptr;
added a commit that references this issue on Jan 14, 2020
7c32597
changed the title [-]CURL_READFUNC_ABORT losed if returned in a middle of transfer[/-] [+]CURL_READFUNC_ABORT is lost if returned in a middle of transfer[/+] on Jan 14, 2020
monnerat

monnerat commented on Jan 15, 2020

@monnerat
Contributor

Function readback_part() tries to fill the buffer as much as it can, calling the read callback function (via read_part_content()) several times if needed.

As soon as the callback returns a non-positive byte count, the loop is exited. If some data were read since function entry, the count is returned to process them. Else the end code.
In the current case, I presume we accumulated some read bytes before CURL_READFUNC_ABORT is returned. Thus these bytes should be processed before abort. However this abort status is not latched and the read callback is expected to return CURL_READFUNC_ABORT again on the very next call (as it probably will return 0 again for EOF or READ_ERROR, etc). In other words, the callback should insist.

here i see random non zero 'cursize' values (is it bytes transfered already before we abort?)

This is not random: this is the effective number of bytes read since readback_part() entry.
That would probably mean: the read callback does not persist returning CURL_READFUNC_ABORT on subsequent calls, but returns read byte counts before re-requesting to abort.

Maybe we should latch the last read size/status to handle this persistence in libcurl itself rather than in the read callback and thereby avoid calling the latter again? If we do that, resetting the CURL_READFUNC_PAUSE state will probably be difficult.

MrdUkk

MrdUkk commented on Jan 15, 2020

@MrdUkk
Author

Yes, it's exactly like @bagder says: my callback is called repeatedly (3-4 times observed in debugger).
perhaps API documentation need clarify this behaviour (user callback MAYBE called repeatedly and should return multiple times CURL_READFUNC_ABORT).
For example only: libmicrohttpd use callbacks too. but it won't call twice (or more) user-callback function in case we ask to abort transfer.

my project using multithreading. thread1 uses curl_easy interface doing data transfer and thread2 posting messages to thread1 (with data).
when posting WAIT_OBJECT_0, thread1 should stop. but it won't. i can rewrote this part. i only ask is this a expected behaviour in libcurl API.

static size_t ReadMemoryCallback(void *contents, size_t size, size_t nmemb, void *userp)
{
	SharedBuffer *ptr = (SharedBuffer *)userp;
	size_t realsize = size * nmemb;
	for (;;)
	{
		DWORD PE = WaitForMultipleObjects(2, ptr->PEvent, FALSE, INFINITE);
		if (PE == WAIT_OBJECT_0)
			return CURL_READFUNC_ABORT;
		else if (PE == WAIT_OBJECT_0 + 1)
		{
			char *ab=ptr->TryDequeune();
			if (!ab) continue;
			memcpy(contents, ab, realsize);
			return realsize;
		}
	}
	return 0;
}
bagder

bagder commented on Jan 15, 2020

@bagder
Member

perhaps API documentation need clarify this behavior (user callback MAYBE called repeatedly and should return multiple times CURL_READFUNC_ABORT).

The callback should not get called again within the same transfer, if it returned ABORT previously. It is a bug.

bagder

bagder commented on Jan 15, 2020

@bagder
Member

Maybe we should latch the last read size/status to handle this persistence in libcurl itself rather than in the read callback and thereby avoid calling the latter again?

Yes. An ABORT from the callback really means abort and it should not be called again. (What if the second call would return something else?) Calling it again also means that the code doesn't actually abort but somehow still tries to continue.

If we do that, resetting the CURL_READFUNC_PAUSE state will probably be difficult.

Difficult maybe, but that's how the API should behave. The callback should only have to return PAUSE once.

10 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @bagder@monnerat@jay@MrdUkk

      Issue actions

        CURL_READFUNC_ABORT is lost if returned in a middle of transfer · Issue #4813 · curl/curl