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

curl_multibyte: always return a heap-allocated copy of string #6602

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Feb 13, 2021

  • 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 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

Copy link
Member

@MarcelRaad MarcelRaad left a 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?

@ghost
Copy link

ghost commented Feb 15, 2021

Congratulations 🎉. DeepCode analyzed your code in 9.778 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@jay
Copy link
Member Author

jay commented Feb 15, 2021

Doesn't build yet, but I like the usage simplification. Can't curlx_unicodefree just be removed completely?

Ok I replaced the calls with Curl_safefree. I don't understand why (free) was used in the curlx_unicodefree macro instead of free, is there a reason to avoid a function-like macro version of free?

@MarcelRaad
Copy link
Member

I don't understand why (free) was used in the curlx_unicodefree macro instead of free, is there a reason to avoid a function-like macro version of free?

There was some CI failure after using the macro for non-Windows too, but I don't remember the details.

@jay
Copy link
Member Author

jay commented Feb 18, 2021

I don't understand why (free) was used in the curlx_unicodefree macro instead of free, is there a reason to avoid a function-like macro version of free?

There was some CI failure after using the macro for non-Windows too, but I don't remember the details.

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.

- 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
Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

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

👍

@sergio-nsk
Copy link
Contributor

sergio-nsk commented Apr 22, 2021

It seems this change causes the NTLM authentication to crash. Crashes happen in Curl_auth_build_spn -> _CrtIsValidHeapPointer.

jay added a commit to jay/curl that referenced this pull request Apr 23, 2021
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
@jay
Copy link
Member Author

jay commented Apr 23, 2021

It seems this change causes the NTLM authentication to crash. Crashes happen in Curl_auth_build_spn -> _CrtIsValidHeapPointer.

Please try #6938

jay added a commit to jay/curl that referenced this pull request Apr 23, 2021
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
@sergio-nsk
Copy link
Contributor

#6938 fixes the issue I have commented. Thank you!

jay added a commit that referenced this pull request Apr 27, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants