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

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

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot 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 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

@MarcelRaad MarcelRaad commented Feb 15, 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.

@jay jay force-pushed the jay:improve_multibyte branch from 38071e6 to dda90a0 Feb 18, 2021
@jay
Copy link
Member Author

@jay 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.

@jay jay force-pushed the jay:improve_multibyte branch from dda90a0 to e59dea7 Feb 18, 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 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
@jay jay force-pushed the jay:improve_multibyte branch from e59dea7 to ce0e186 Feb 18, 2021
Copy link
Member

@MarcelRaad MarcelRaad left a comment

👍

@jay jay closed this in 0936350 Feb 20, 2021
@jay jay deleted the jay:improve_multibyte branch Feb 20, 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

2 participants