Skip to content

Use a custom implementation of wcsdup on Windows, so that malloc/free…#7540

Closed
Myriachan wants to merge 3 commits intocurl:masterfrom
Myriachan:myriachan/win32-wcsdup-fix
Closed

Use a custom implementation of wcsdup on Windows, so that malloc/free…#7540
Myriachan wants to merge 3 commits intocurl:masterfrom
Myriachan:myriachan/win32-wcsdup-fix

Conversation

@Myriachan
Copy link
Contributor

… overrides from curl_global_init are used for wcsdup correctly.

… overrides from curl_global_init are used for wcsdup correctly.
lib/strdup.c Outdated
size_t lengthnul;
size_t lengthbytes;

if (SIZE_MAX - length < 1)
Copy link
Contributor Author

@Myriachan Myriachan Aug 6, 2021

Choose a reason for hiding this comment

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

These overflow checks are probably unnecessary, because presumably the wchar string has a size fitting in size_t. I don't know what your standards are for checks like these.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

stdint may not be available and shouldn't be necessary. curl_setup defines SIZE_T_MAX, you can use that instead. Also Curl_wcsdup doesn't need so many lines, you can do a single check like
if(length > (SIZE_T_MAX / sizeof(wchar_t)) - 1)
return NULL;
return (wchar_t *)Curl_memdup(src, (length + 1) * sizeof(wchar_t))

@jay jay added Windows Windows-specific crash labels Aug 7, 2021
@jay
Copy link
Member

jay commented Aug 7, 2021

@Myriachan
Copy link
Contributor Author

stdint may not be available and shouldn't be necessary. curl_setup defines SIZE_T_MAX, you can use that instead. Also Curl_wcsdup doesn't need so many lines, you can do a single check like

Thanks! Didn't know about libcurl having a SIZE_T_MAX. Also, I didn't know how wordy the coding style should be.

@Myriachan Myriachan marked this pull request as ready for review August 7, 2021 06:38
@bagder bagder requested a review from jay August 8, 2021 20:42
@bagder
Copy link
Member

bagder commented Aug 9, 2021

Thanks!

@bagder bagder closed this in 76e047f Aug 9, 2021
@bagder bagder removed the request for review from jay August 9, 2021 12:13
@lukellmann
Copy link

_wcsdup is still used in the global_init function in easy.c. This means the wrong function will be used after reinitialisation after a full cleanup:

// sets 'Curl_cwcsdup' to '_wcsdup'
curl_global_init(CURL_GLOBAL_DEFAULT);
curl_global_cleanup();

// doesn't reset 'Curl_cwcsdup' to 'Curl_wcsdup', it's still '_wcsdup'
curl_global_init_mem(CURL_GLOBAL_DEFAULT, m, f, r, s, c);

@jay
Copy link
Member

jay commented Sep 28, 2024

If you have found a bug in curl related to this PR then please open a new issue and explain how to reproduce

vszakats added a commit to vszakats/curl that referenced this pull request Jul 7, 2025
Also:
- add two related casts to match rest of code.
- replace deprecated `wcsdup` with `_wcsdup` in libtests.
  Ref: https://learn.microsoft.com/cpp/c-runtime-library/reference/strdup-wcsdup

Follow-up to 76e047f curl#7540
Bug: curl#17840 (comment)
Bug: curl#7540 (comment)

Closes curl#17840
vszakats added a commit to vszakats/curl that referenced this pull request Jul 7, 2025
Also:
- add two related casts to match rest of code.
- replace deprecated `wcsdup` with `_wcsdup` in libtests.
  Ref: https://learn.microsoft.com/cpp/c-runtime-library/reference/strdup-wcsdup

Follow-up to 76e047f curl#7540
Bug: curl#17840 (comment)
Bug: curl#7540 (comment)

Closes curl#17840
vszakats added a commit that referenced this pull request Jul 8, 2025
This callback was permanently mapped to libcurl's internal
`Curl_wcsdup()`, which always uses the customizable malloc for
allocation, thus making a custom mapping redundant anyway.

To simplify, drop the callback and map `_tcsdup()` in Unicode mode
directly to `Curl_wcsdup()`.

Also fixes:
- `curl_global_init()` which, before this patch, (re)initialized its
  mapping to `_wcsdup()`, returning buffers potentially incompatible
  with a custom allocator.
  Bug: #17840 (comment)
  Bug: #7540 (comment)
  Co-reported-by: Luca Kellermann

Follow-up to 76e047f #7540
Assisted-by: Jay Satiro

Closes #17843
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crash Windows Windows-specific

Development

Successfully merging this pull request may close these issues.

5 participants