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

ldap: use correct memory free function #6671

Closed
wants to merge 1 commit into from

Conversation

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Feb 26, 2021

unescaped is coming from Curl_urldecode and not a unicode conversion function, so reclaiming its memory should be performed with a normal call to free rather than curlx_unicodefree. In reality, this is the same thing as curlx_unicodefree is implemented as a call to free but that's not guaranteed to always hold (and is less readable in this case IMO).

@jay
jay approved these changes Feb 26, 2021
Copy link
Member

@jay jay left a comment

curlx_unicodefree uses (free) with parentheses so it is not overridden by the memory debugger free macro. in other words the curlx functions are not meant to be memory tracked, afaict

unescaped is coming from Curl_urldecode and not a unicode conversion
function, so reclaiming its memory should be performed with a normal
call to free rather than curlx_unicodefree.  In reality, this is the
same thing as curlx_unicodefree is implemented as a call to free but
that's not guaranteed to always hold.  Using the curlx macro present
issues with memory debugging as well.

Closes #6671
Reviewed-by: Jay Satiro <raysatiro@yahoo.com>
@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-ldap_free branch from 11b1fae to afc0ab7 Feb 28, 2021
@danielgustafsson
Copy link
Member Author

@danielgustafsson danielgustafsson commented Feb 28, 2021

Found two more instances so pushed a small rebase with those for another just-in-case run through the CI farm before landing this is in master.

@bagder
bagder approved these changes Mar 1, 2021
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

3 participants