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

easy: fix memory access in 'easy_perform' #5363

Closed
wants to merge 6 commits into from
Closed

Conversation

@denzor200
Copy link
Contributor

denzor200 commented May 9, 2020

No description provided.

@emilengler
Copy link
Contributor

emilengler commented May 9, 2020

Could you please give a more detailed description about the problem it fixes? This makes reviewing much easier

@denzor200
Copy link
Contributor Author

denzor200 commented May 9, 2020

Could you please give a more detailed description about the problem it fixes? This makes reviewing much easier

So, let's to see 'curl_multi_add_handle' error-handling tree.
In this case, 'curl_multi_cleanup(multi);' called always, even if we don't allocate new memory in this perform-iteration.
After cleanup-call the memory had freed, but the pointer still links to this dead memory..
Note, that it is not a temporary poiner - it'a pointer inside context structure

@bagder
Copy link
Member

bagder commented May 10, 2020

Can you please give us a recipe that reproduces this problem you want to fix like this? Or at least explain with more details how the unpatched code behaves wrongly? We run all the tests with torture tests all the time, I don't see how for example a failed alloc can trigger any problem there.

@denzor200
Copy link
Contributor Author

denzor200 commented May 10, 2020

Can you please give us a recipe that reproduces this problem you want to fix like this? Or at least explain with more details how the unpatched code behaves wrongly? We run all the tests with torture tests all the time, I don't see how for example a failed alloc can trigger any problem there.

I don't have recipes for it, but it's a potential problem only if any libcurl-using software wants to continue after CURLcode handling.
You can refuse, maybe 'libcurl' not designed for very reliable software - i don't know it.

@bagder
Copy link
Member

bagder commented May 10, 2020

You can refuse, maybe 'libcurl' not designed for very reliable software - i don't know it.

That attitude won't help anyone. libcurl is very reliable software and as such we cannot accept code changes into the project that isn't well motivated. You propose a change here but you cannot explain to us why it is needed. We cannot merge changes that nobody sees a point with and not even the author of it can explain what it fixes.

@jay
Copy link
Member

jay commented May 11, 2020

Can you please give us a recipe that reproduces this problem you want to fix like this? Or at least explain with more details how the unpatched code behaves wrongly? We run all the tests with torture tests all the time, I don't see how for example a failed alloc can trigger any problem there.

curl_multi_add_handle doesn't fail due to lack of memory which is why torture test isn't catching this. His analysis is technically correct even though the outcome doesn't seem possible unless caused by user error, such as multi threading without sync. Anyway it's not good practice to leave it that way. @denzor200 why not just set it to NULL?

diff --git a/lib/easy.c b/lib/easy.c
index 3cb3579..988ff61 100644
--- a/lib/easy.c
+++ b/lib/easy.c
@@ -681,6 +681,7 @@ static CURLcode easy_perform(struct Curl_easy *data, bool events)
   mcode = curl_multi_add_handle(multi, data);
   if(mcode) {
     curl_multi_cleanup(multi);
+    data->multi_easy = NULL;
     if(mcode == CURLM_OUT_OF_MEMORY)
       return CURLE_OUT_OF_MEMORY;
     return CURLE_FAILED_INIT;
@bagder
Copy link
Member

bagder commented May 11, 2020

  • data->multi_easy = NULL;

Ah yes that seems like the right defensive thing to do, even as you say it's really hard to get curl_multi_add_handle() to return an error there.

@denzor200
Copy link
Contributor Author

denzor200 commented May 11, 2020

why not just set it to NULL?

I agree, a good idea

@bagder
Copy link
Member

bagder commented May 11, 2020

@denzor200 will you make your PR do that instead and rebase/force-push?

denzor200 added 2 commits May 11, 2020
This reverts commit b338be2.
@jay jay closed this in a902171 May 12, 2020
@jay
Copy link
Member

jay commented May 12, 2020

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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