Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
MrdUkk opened this issue Jan 14, 2020 · 13 comments
Closed

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

MrdUkk opened this issue Jan 14, 2020 · 13 comments
Labels

Comments

@MrdUkk
Copy link

@MrdUkk MrdUkk commented Jan 14, 2020

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()

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 14, 2020

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

@jay

This comment has been minimized.

Copy link
Member

@jay jay commented Jan 14, 2020

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Member

@jay jay commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 14, 2020

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;
bagder added a commit that referenced this issue Jan 14, 2020
When *ABORT is returned from the callback, the transfer must stop
immediately and the return code must be passed back to the parent all
the way up to Curl_fillreadbuffer().

Test 644 was supposed to verify this but it wasn't done right. Now it
makes sure the callback is never called again after CURL_READFUNC_ABORT
is returned and it also makes sure the data transfer is actually
stopped as instructed.

Also removed the 'flaky' keyword since the test is now modified.

Fixes #4813
Reported-by: MrdUkk on github
@bagder bagder 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 Jan 14, 2020
@monnerat

This comment has been minimized.

Copy link
Contributor

@monnerat monnerat commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Author

@MrdUkk MrdUkk commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Member

@bagder bagder commented Jan 15, 2020

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.

@MrdUkk

This comment has been minimized.

Copy link
Author

@MrdUkk MrdUkk commented Jan 16, 2020

i tested old libcurl (version 7.39.0) -works like a charm. my callback is called once.

monnerat added a commit to monnerat/curl that referenced this issue Jan 19, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
monnerat added a commit to monnerat/curl that referenced this issue Jan 20, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
monnerat added a commit to monnerat/curl that referenced this issue Jan 21, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
@MrdUkk

This comment has been minimized.

Copy link
Author

@MrdUkk MrdUkk commented Jan 27, 2020

@MrdUkk MrdUkk closed this Jan 27, 2020
monnerat added a commit to monnerat/curl that referenced this issue Jan 29, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
bagder added a commit that referenced this issue Mar 1, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes #4813
Reported-by: MrdUkk on github
Closes #4833
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Mar 2, 2020

Reverted the merge, this issue is still open.

@bagder bagder reopened this Mar 2, 2020
monnerat added a commit to monnerat/curl that referenced this issue Mar 3, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
monnerat added a commit to monnerat/curl that referenced this issue Mar 6, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
monnerat added a commit to monnerat/curl that referenced this issue Mar 6, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
monnerat added a commit to monnerat/curl that referenced this issue Mar 7, 2020
In case a read callback returns a status (pause, abort, eof,
error) instead of a byte count, drain the bytes read so far but
remember this status for further processing.
Takes care of not losing data when pausing, and properly resume a
paused mime structure when requested.
New tests 670-673 check unpausing cases, with easy or multi
interface and mime or form api.

Fixes curl#4813
Reported-by: MrdUkk on github
@bagder bagder closed this in 96972ec Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.