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

Using curl_formadd to post file could not rewind correctly when redirection happens #11680

Closed
yushicheng7788 opened this issue Aug 16, 2023 · 10 comments

Comments

@yushicheng7788
Copy link

yushicheng7788 commented Aug 16, 2023

I did this

I am using curl_formadd to post MIME data to server, and server replies redirection. But it seems libcurl didn't rewind correctly.
I think this phenomenon was cause by this commit 1b39731
In the version before 7.87.0, libcurl will do rewind immediately, but in version 7.87.0, libcurl will only make a mark, and then do the rewind operation in the next round of state machine driving. But at that time, the cached req member seems to be reinitialized again, causing http->sendit to be empty, and when sending data with curl_formadd, mimepost is also empty, and rewind is not executed correctly.

I expected the following

I'm wondering if my understanding is correct or not. Would like get your confirmation.

curl/libcurl version

curl 8.1.2

operating system

Windows x86_64

@bagder bagder self-assigned this Aug 16, 2023
bagder added a commit that referenced this issue Aug 16, 2023
The data is kept for the entire transfer and not only per single HTTP
request. This re-enables rewind in the beginning of the second request
instead of in end of the first, as brought by 1b39731.

Fixes #11680
Reported-by: yushicheng7788 on github
@bagder
Copy link
Member

bagder commented Aug 16, 2023

It looks like an entirely correct analysis. I made #11682 as a first attempt at a fix.

bagder added a commit that referenced this issue Aug 16, 2023
The data is kept for the entire transfer and not only per single HTTP
request. This re-enables rewind in the beginning of the second request
instead of in end of the first, as brought by 1b39731.

Fixes #11680
Reported-by: yushicheng7788 on github
@yushicheng7788
Copy link
Author

Thank you very much for your reply, but we found a crash after applying your patch. Libcurl crashed at
res = part->seekfunc(part->arg, (curl_off_t) 0, SEEK_SET); in mime.c file, seekfunc ptr becomes a wild pointer. I found that before deciding to do redirection(Curl_follow), libcurl will execute the multi_done function, which will execute to curl_http_done, thereby cleaning up the resources pointed to by your newly added formp ptr, formp and data->state.mimepost point to the same address, and then cause seekfunc be a wild pointer.

@bagder
Copy link
Member

bagder commented Aug 17, 2023

Can you show me a small example code that reproduces this problem so that I can better understand what's going on (and write a test case to repro) ?

bagder added a commit that referenced this issue Aug 17, 2023
The data is kept for the entire transfer and not only per single HTTP
request. This re-enables rewind in the beginning of the second request
instead of in end of the first, as brought by 1b39731.

Extend test 650

Fixes #11680
Reported-by: yushicheng7788 on github
Closes #11682
@yushicheng7788
Copy link
Author

I'm very sorry, I can't provide detailed code due to privacy issues, but the specific details can be summarized as using curl_formadd() to send files, the server needs to return 307 redirection. The backbone of the code is shown below.

CURL* curl;
CURLcode res;
curl_global_init(CURL_GLOBAL_ALL);
curl = curl_easy_init();
if (curl) {
  std::string url;
  curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
  struct curl_httppost* m_formpost;
curl_formadd(&m_formpost, &lastptr,
	CURLFORM_COPYNAME, fieldName.c_str(),
	CURLFORM_STREAM, (void*)fileStream,
	CURLFORM_CONTENTSLENGTH, (long)fileStream->GetTotalSize(),
	CURLFORM_FILENAME, fileName.c_str(),
	CURLFORM_CONTENTTYPE, contentType.c_str(),
	CURLFORM_END);
  curl_easy_setopt(curl, CURLOPT_HTTPPOST, m_formpost);
  res = curl_easy_perform(curl);
  if (res != CURLE_OK)
    fprintf(stderr, "curl_easy_perform() failed: %s\n",
      curl_easy_strerror(res));
  curl_easy_cleanup(curl);
}
curl_global_cleanup();
return 0;

Perhaps I can share some of my findings with you. When the state machine is in the MSTATE_DO state, the multi_do function will store the form data in data->state.formp, data->state.formp will also make a copy to data->state.mimepost, and then the state machine will transfer to the MSTATE_DID state . This state will be further transferred to the MSTATE_PERFORMING state. In this state, if the location field is found in the response header, it will be redirected (data->req.newurl not NULL). But before the state machine transfers to MSTATE_CONNECT, the multi_done function will be executed, which will clean up data->state.formp. But data->state.formp and data->state.mimepost share the same piece of memory, and data->state.mimepost will be used in rewind, but the resources it points to have been released, resulting in wild pointers. Hope the clues I found can help you. Thanks.

@bagder
Copy link
Member

bagder commented Aug 17, 2023

I improved #11682 a bit after your first comment and did a test to verify. It probably does not fully mimic your case but did it improve things for you?

@yushicheng7788
Copy link
Author

Thank you for your submission. There is no crash now, but I still have some questions to ask you, I didn't see where do data->state.formp initialization, according to my understanding, it will be a wild pointer, so when do setopt operation, execute Curl_mime_cleanpart(data->state.formp); will there be risks?

@bagder
Copy link
Member

bagder commented Aug 17, 2023

I didn't see where do data->state.formp initialization

The CURLOPT_HTTPPOST option is the old/legacy way of doing formposts. In the Curl_http_body() function, we convert the old style into the new:

curl/lib/http.c

Lines 2382 to 2392 in 1662cc2

if(!data->state.formp) {
data->state.formp = calloc(sizeof(curl_mimepart), 1);
if(!data->state.formp)
return CURLE_OUT_OF_MEMORY;
Curl_mime_cleanpart(data->state.formp);
result = Curl_getformdata(data, data->state.formp, data->set.httppost,
data->state.fread_func);
if(result)
return result;
data->state.mimepost = data->state.formp;
}

The data is allocated and state.mimepost is also made to point to the state.formp data. This data is kept until the handle is closed again (in Curl__close()) or when CURLOPT_HTTPPOST is set again.

@yushicheng7788
Copy link
Author

Thank you for your answer, we will consider using a new way to do formpost. But when setopt, formp should not be initialized? So won't Curl_mime_cleanpart access wild pointers?

case CURLOPT_HTTPPOST:
    /*
     * Set to make us do HTTP POST
     */
    data->set.httppost = va_arg(param, struct curl_httppost *);
    data->set.method = HTTPREQ_POST_FORM;
    data->set.opt_no_body = FALSE; /* this is implied */
    Curl_mime_cleanpart(data->state.formp);
    Curl_safefree(data->state.formp);
    break;

@bagder
Copy link
Member

bagder commented Aug 17, 2023

It is cleared. That's what Curl_safefree does.

@yushicheng7788
Copy link
Author

Thanks again for your response, I've tested it and it's working fine, excellent work!

@bagder bagder closed this as completed in 74b87a8 Aug 17, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this issue Sep 25, 2023
When the legacy CURLOPT_HTTPPOST option is used, it gets converted into
the modem mimpost struct at first use. This data is (now) kept for the
entire transfer and not only per single HTTP request. This re-enables
rewind in the beginning of the second request instead of in end of the
first, as brought by 1b39731.

The request struct is per-request data only.

Extend test 650 to verify.

Fixes curl#11680
Reported-by: yushicheng7788 on github
Closes curl#11682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants