-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
curl_multibyte: always return a heap-allocated copy of string #6602
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't build yet, but I like the usage simplification. Can't curlx_unicodefree
just be removed completely?
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
Ok I replaced the calls with Curl_safefree. I don't understand why |
There was some CI failure after using the macro for non-Windows too, but I don't remember the details. |
38071e6
to
dda90a0
Compare
So it appears I can't get rid of curlx_unicodefree. Since the conversion functions are curlx they are not tracked by memdebug which is why that (free) was in parentheses. I've reverted the change to Curl_safefree and added comments explaining why the function-like macros in multibyte use parentheses. |
dda90a0
to
e59dea7
Compare
- Change the Windows char <-> UTF-8 conversion functions to return an allocated copy of the passed in string instead of the original. Prior to this change the curlx_convert_ functions would, as what I assume was an optimization, not make a copy of the passed in string if no conversion was required. No conversion is required in non-UNICODE Windows builds since our tchar strings are type char and remain in whatever the passed in encoding is, which is assumed to be UTF-8 but may be other encoding. In contrast the UNICODE Windows builds require conversion (wchar <-> char) and do return a copy. That inconsistency could lead to programming errors where the developer expects a copy, and does not realize that won't happen in all cases. Closes #xxxx
e59dea7
to
ce0e186
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
It seems this change causes the NTLM authentication to crash. Crashes happen in Curl_auth_build_spn -> _CrtIsValidHeapPointer. |
Please try #6938 |
curlx_convert_UTF8_to_tchar must be freed by curlx_unicodefree, but prior to this change some uses mistakenly called free. I've reviewed all other uses of curlx_convert_UTF8_to_tchar and curlx_convert_tchar_to_UTF8. Bug: curl#6602 (comment) Reported-by: sergio-nsk@users.noreply.github.com Closes #xxxx
#6938 fixes the issue I have commented. Thank you! |
curlx_convert_UTF8_to_tchar must be freed by curlx_unicodefree, but prior to this change some uses mistakenly called free. I've reviewed all other uses of curlx_convert_UTF8_to_tchar and curlx_convert_tchar_to_UTF8. Bug: #6602 (comment) Reported-by: sergio-nsk@users.noreply.github.com Closes #6938
allocated copy of the passed in string instead of the original.
Prior to this change the curlx_convert_ functions would, as what I
assume was an optimization, not make a copy of the passed in string if
no conversion was required. No conversion is required in non-UNICODE
Windows builds since our tchar strings are type char and already in
UTF-8, so no conversion takes place.
In contrast the UNICODE Windows builds require conversion
(wchar <-> char) and do return a copy. That inconsistency could lead to
programming errors where the developer expects a copy, and does not
realize that won't happen in all cases.
Closes #xxxx