Skip to content

Plug some leaks in curl's proxy handling - #4359

Merged
ethomson merged 4 commits into
masterfrom
cmn/proxy-options-free
Oct 7, 2017
Merged

Plug some leaks in curl's proxy handling#4359
ethomson merged 4 commits into
masterfrom
cmn/proxy-options-free

Conversation

@carlosmn

Copy link
Copy Markdown
Member

I was trying to reproduce the segfault we see in Travis when talking to the proxy, but instead I found a few leaks.

When we duplicate a user-provided options struct, we're stuck with freeing the
url in it. In case we add stuff to the proxy struct, let's add a function in
which to put the logic.
@ethomson

Copy link
Copy Markdown
Member

Neat, thanks for fixing these. Which segfault? The one from with NTLM creds from #4317 and #4321?

@carlosmn

Copy link
Copy Markdown
Member Author

I was looking at #4339 where the thread-unsafe suite seems to be failing somewhat reliably, but I've been unable to reproduce it.

@ethomson

Copy link
Copy Markdown
Member

Oh, that's interesting, isn't it. I hadn't seen that.

Comment thread src/proxy.c Outdated
return 0;
}

void git_proxy_options_free(git_proxy_options *opts)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We usually call this kind of function git_proxy_options_clear instead of free. A free should also free the structure itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, that makes sense. Done.

@ethomson

ethomson commented Oct 7, 2017

Copy link
Copy Markdown
Member

Thanks @carlosmn !

@ethomson
ethomson merged commit 21e6a11 into master Oct 7, 2017
@pks-t pks-t added the backport label Jan 11, 2018
@pks-t pks-t mentioned this pull request Jan 12, 2018
@ethomson
ethomson deleted the cmn/proxy-options-free branch January 9, 2019 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants