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

options: fixes memory leak on API misuse #16599

Closed
wants to merge 2 commits into from

Conversation

catenacyber
Copy link

CURLOPT_PROXYUSERPWD should not be used if
CURLOPT_PROXYUSERNAME and/or CURLOPT_PROXYPASSWORD get used and vice versa

Will allow fuzzers to go past that with curl/curl-fuzzer#124 cc @cmeister2

CURLOPT_PROXYUSERPWD should not be used if
CURLOPT_PROXYUSERNAME and/or CURLOPT_PROXYPASSWORD get used
and vice versa
@@ -2140,6 +2140,10 @@ static CURLcode setopt_cptr(struct Curl_easy *data, CURLoption option,
/*
* user:password needed to use the proxy
*/
if(data->set.str[STRING_PROXYUSERNAME] ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at some of the errors I think this code may need to come directly after the char * lines below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushinf a fixup commit then

@bagder
Copy link
Member

bagder commented Mar 6, 2025

CURLOPT_PROXYUSERPWD should not be used if CURLOPT_PROXYUSERNAME and/or CURLOPT_PROXYPASSWORD get used and vice versa

I don't believe this is true, nor documented.

Can we create a test case first that proves this to be a problem?

bagder added a commit that referenced this pull request Mar 6, 2025
Prevent the previous memory leak. Adjusted test 590 to reproduce the
problem then verify the fix.

Fixes #16599
Reported-by: Catena cyber
@bagder
Copy link
Member

bagder commented Mar 6, 2025

I prefer #16601

@testclutch
Copy link

Analysis of PR #16599 at 2c479d12:

Test 1521 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 93 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@bagder bagder closed this in 4e8d621 Mar 7, 2025
@catenacyber
Copy link
Author

Thanks for making a better fix

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

Successfully merging this pull request may close these issues.

4 participants