windows: drop redundant curl_wcsdup_callback callback

[vszakats]

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: memory: stop overriding unused wcsdup()/_wcsdup() system functions #17840 (comment)
  Bug: Use a custom implementation of wcsdup on Windows, so that malloc/free… #7540 (comment)
  Co-reported-by: Luca Kellermann

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

Closes #17843

[vszakats]

@jay What do you think?

[jay]

re #7540 (comment) it looks like he's correct and I think we should give him credit Reported-by: Luca Kellermann even though he didn't follow up.

Curl_cwcsdup isn't set in curl_global_init_mem presumably because there's no wcsdup callback that the user can set. We probably thought it would be covered since the user sets a malloc callback which is then called by our internal wcsdup (Curl_wcsdup) but wcsdup (defined to Curl_cwcsdup pointer) doesn't always point to Curl_wcsdup. So your fix is right,

#if defined(_WIN32) && defined(UNICODE)
-    Curl_cwcsdup = (curl_wcsdup_callback)_wcsdup;
+    Curl_cwcsdup = (curl_wcsdup_callback)Curl_wcsdup;
#endif

however it makes that line redundant. It's already set before main so you could just remove those 3 lines instead. Another way that may be more palatable is leave the line in curl_global_init unchanged and add the line to curl_global_init_mem instead. There may be some slight performance benefit to have curl_global_init (ie no user memory callbacks) continue to call _wcsdup directly. Possibly though I'm forgetting something like trackmemory needs to track it. So 3 different ways to do it, one of the ways is the most correct but I'm not sure which.

[vszakats]

In theory someone might rely on this reset behavior if setting Curl_cwcsdup
behind the back of libcurl as described in https://curl.se/mail/lib-2021-07/0056.html.
Such hack isn't necessary after #7540, and the reset code was wrong, still,
IMO it'd be nicer to reset it from global_init(), for correctness.

I think setting it to _wcsdup would not work, because a customized allocator
could free a pointer allocated by the standard malloc called from _wcsdup.

I'm now wondering if it'd be possible to just #define _tcsdup Curl_wcsdup in
Unicode builds, and drop Curl_cwcsdup completely? Since we don't allow
overriding it, and there is also no use of doing that, because it internally calls into
the custom malloc anyway.

Credits: The original report remained hidden until I bumped into this issue while
working on #17840, to find out about the first report while reading up the code's
history. It was still useful to reinforce the problem, but like you mentioned in #7540,
it's important to report it as a new issue. Does "Co-reported-by" sound OK?

[vszakats]

Dropping the Windows-specific, redundant, callback, seems to work fine.

[jay]

Dropping the Windows-specific, redundant, callback, seems to work fine.

Ah the fourth way. Yes, that works :)

Does "Co-reported-by" sound OK?

Sure, however you think is best.