curlx: move fopen and multibyte to curlx_malloc

[bagder]

Let's see what fallout this makes

[icing]

CI is so good, you can see what you break!

[vszakats]

I guess this would break debug builds because these calls are
used from the memdebug wrappers. I think it was original reason
for using the raw allocators here.

[bagder]

So why do we need to fopen() magic in the memdebug code? It's just debug code, we can keep it simple can we not?

[vszakats]

So why do we need to fopen() magic in the memdebug code? It's just debug code, we can keep it simple can we not?

It's saving files to disk, and saving files to disk involves dealing with
long filenames, unicode. On Windows simple things are not simple.

[bagder]

So why do we need to fopen() magic in the memdebug code? It's just debug code, we can keep it simple can we not?

It's saving files to disk, and saving files to disk involves dealing with long filenames, unicode. On Windows simple things are not simple.

This is debug code - we control the entire thing. We don't need to accept unicode and weird stuff here.

[vszakats]

Then is creates and exception, which needs to be kept in mind and dealt all the time.

What would be the goal of using the debug/curl allocators in these low-level functions?
They have never been used there before.

[bagder]

I think my main point is that the malloc() replacement feels more important since they are used in production code, while the memdebug code is primarily for us to debug curl with and we use those in more controlled environments and it feels easy enough to apply a few restrictions there, if the benefit is that we can do the mallocs correctly.

[bagder]

They have never been used there before.

That was a bug. We offer users the ability to replace all libcurl mallocs to instead use their allocation functions.

[bagder]

Then is creates and exception

This just reverses the exception. Instead of accepting malloc() in production code this has to accept fopen and freopen in debug code.

[bagder]

@vszakats are these windows CI failures due to this change? They look odd!

[vszakats]

They seem to be crashing on curl -V.

Forgot to say thet memdebug functions also wrap fopen/open functions
and it'd be unfortunate if they'd change behavior compared to the original,
in addition to doing their memdebug things.

These allocators are fine. They could probably be updated to use
win32-specific allocators, if the optics of this exception is not good, but
we still cannot use the curlx_ ones if we want to avoid recursion.

[bagder]

They could probably be updated to use win32-specific allocators,

That would not help though. I want us to

A) honor the curl_global_init_mem() function as well as possible. The user then provides their set of allocation functions to replace the ones libcurl uses. We therefore need to use the correct set to have this work.

B) for debug builds, I want the memdebug system to trace all the memory calls we do in curl + libcurl, which then also requires us to use the correct set

[vszakats]

I'd argue these goals aren't worth the mess it may need to support
them, though this is of course subjective. I'm also silently
celebrating that I could untangle allocators after many false-starts
and a lot of work, and not so keen today to re-add even a small bit
of a mess.

For fopen, these Windows-specific allocations are self-contained,
the code is mature and not expected to change. We saw nobody
complaining they aren't using custom allocators.

For the two multibyte ones, and their unicodefree pair, memdebug
may theoretically discover issues. As for custom allocators, nobody
complained either, and these pointers (AFAIU) should never be
returned by a libcurl API. They are private to libcurl, thus unlikely to
bother most API users how they are allocated. Unless for stats or
rare use-cases.

But, I also have no idea how to achieve these goals without making
more of a mess to counteract the benefits.

fopen/open can be dumbed down for debug-enabled builds, which
in turn will cause great pain down the line, because they'll behave
differently than normal builds. They would also expose the messy
Windows open functions to more code, also something I just cleaned
up.

For the unicode functions: Unicode barely works in curl, so perhaps
it could all be just deleted and replaced with the unobstrusive solution
that requires Win10. Breaking or disallowing Unicode for debug-enabled
builds, I'd find super problematic, to wrap one's head around when
things start breaking.

A duplicate copy of these functions could be kept that's exclusively
used from memdebug, and keep using standard allocators. Which
would mean to support 2 copies of this in slightly different variants
from now on. I'm not a fan of that.

Finally, just to make it clear because I feel this is now suddenly on
me: This had always worked liked this, and I merely kept it up to not
break things.

And as always: it's more than likely I'm missing something.

[bagder]

I feel this is now suddenly on me

I don't think it is on you nor "your problem". I think it is just like always. Iteratively taking steps forward. You took this a big step forward which then enabled me to spot and realize this particular detail that I would like to fix.

[vszakats]

I feel this is now suddenly on me

I don't think it is on you nor "your problem". I think it is just like always. Iteratively taking steps forward. You took this a big step forward which then enabled me to spot and realize this particular detail that I would like to fix.

Phew, thanks for clarifying! It's good it's easier to oversee what's
happening.

IMO it'd be worth considering to declare defeat for the "WIDE"
Unicode interface, and focus on UTF-8.

But! According to comments, GetFullPathNameW() is required
for long-filename support, raising the question of how to support
long-filenames in UTF-8 mode. Maybe it's automatically done by
GetFullPathNameA() in that case?

For non-Windows multibye, I'm trying to map them to the curlx
ones in #19751.

[vszakats]

curlx_convert_UTF8_to_wchar and curlx_convert_wchar_to_UTF8 are
used only in lib/idn.c outside curlx. Since the max input string length is
255 chars, it seems feasible to replace them with MultiByteToWideChar
and WideCharToMultiByte and stack buffers.

[bagder]

Since the max input string length is 255 chars

In idn.c it might have that limit, but the function is also used for filenames in fopen.c and I figure they can be (much) larger than 255.

[vszakats]

Since the max input string length is 255 chars

In idn.c it might have that limit, but the function is also used for filenames in fopen.c and I figure they can be (much) larger than 255.

Sorry, I meant to say to replace the curlx calls in idn.c with direct MultiByte*
functions.

This would reduce raw memallocs, a bit. It'd also remove the last uses of the
two curlx multibyte functions outside curlx, and allow to define them for
Windows Unicode builds only.

[bagder]

Sorry, I meant to say to replace the curlx calls in idn.c with direct MultiByte* functions.

Ah right. Yeah that would simplify things a little!

[vszakats]

Made #19845 to fix this. Reduces system allocator use to Windows
TrackMemory builds in the 4 fopen/stat wrappers. Buffers are freed
within these 4 functions.