curlx: replace mbstowcs/wcstombs with _s counterparts (Windows)

[vszakats]

They are used in Windows-specific fopen(), freopen, open() and
curlx_get_winapi_error() calls, and in fix_excessive_path() in
Unicode builds.

Refs:
https://learn.microsoft.com/cpp/c-runtime-library/reference/mbstowcs-mbstowcs-l
https://learn.microsoft.com/cpp/c-runtime-library/reference/mbstowcs-s-mbstowcs-s-l
https://learn.microsoft.com/cpp/c-runtime-library/reference/wcstombs-wcstombs-l
https://learn.microsoft.com/cpp/c-runtime-library/reference/wcstombs-s-wcstombs-s-l

Also ban these functions via checksrc.

Co-authored-by: Jay Satiro

---

https://github.com/curl/curl/pull/19581/files?w=1

• rebase on adding curlx_freopen() → curlx: add and use curlx_freopen() #19598
• rebase on lib: replace _tcsncpy/wcsncpy/wcscpy with _s counterparts (Windows) #19589
• upgrade call in winapi.c.

[jay]

IMO changes like this are unnecessary. I've never been a fan of these _s suffixed "secure" functions. This is harder to understand and for multibyte conversion code changes it looks like it doesn't have the same effect. These secure functions invoke what is basically an exception if the conversion fails however I wrote the code so it could fail and would not truncate. In other words, in the normal course of business functions like this should be able to fail without interrupting (crashing) the application. If the path can't be fixed for that reason, which is certainly possible, then it can't be fixed. That's OK.

edit: It looks like the invalid parameter handler is not invoked if the conversion fails. Still don't like it though.

[vszakats]

@jay: Thanks for your fixes! I also don't like the _s functions and they are not
fixing a security/safety issue in these particular cases. But I figure why not use
them if supported platforms are compatible with them, for a peace of mind and
to avoid these warnings that we need to actively silence (for decades now).
These changes are also limited to a small area of code, which seems fine to me.
Returning errno instead of altering the globals is a small improvement.

[vszakats]

Tests still hang due to file open errors, so something is fishy with my open changes.

[rsbeckerca]

Are the methods generally portable? I have not seen _s for wcs broadly.

[vszakats]

Are the methods generally portable? I have not seen _s for wcs broadly.

They are supported by all the compiler/SDK versions curl supports.
The CI tests the complete supported range, and they build correctly
with these updates.

I didn't explore the edges of portability further than the supported range.

[vszakats]

Moved the first commit here to a separate PR: #19589

[vszakats]

I wrongly thought "safe" open functions leave the global errno alone. They do not:

"Always check the return value to see if the function succeeded before you do
any further operations on the file. If an error occurs, the error code is returned,
and the global variable errno is set."
Ref: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-s-wfopen-s?view=msvc-170#remarks

[vszakats]

@rsbeckerca I checked mbstowcs_s, and it's supported by VS2005 and up [1], and all mingw-w64 versions according to its 1.0 headers, referencing MSVCRT.

source (Chromium dependency): https://github.com/google/breakpad/blob/470771e32d0167c2654ecb9113dc56a3510b695f/src/common/windows/string_utils.cc#L59-L69

The few other ones I checked randomly are also present in msvcrt.dll. TL;DR these APIs look fairly old.

[jay]

It seems harder to maintain, people reading the code including myself may not be familiar. Everyone who writes C can probably head parse a fopen but not a fopen_s, strncpy but not strncpy_s, etc. Regarding the warnings though, are you referring to deprecation warnings? MS "deprecation" for stuff like this is basically they aren't going to remove the functions:

In this context, "deprecated" means that use of the function isn't recommended. It doesn't mean the function will be removed from the CRT.

If those are the warnings you mean I suggest silence them with _CRT_SECURE_NO_WARNINGS etc.

The reason I took issue with security is because IMO it's only an improvement in some limited circumstances that aren't really conducive to the way we all write. There's no reason for a strncpy_s to receive the size of the destination buffer because the size being copied is already passed as the count. Here's an example from the pr:

if(mbstowcs_s(&count, ibuf, needed, in, needed - 1))

If we calculated needed wrong, then needed - 1 would also be wrong, that's sort of inescapable. They are both based on the same calculation of needed bytes characters, one with nul and one without. That's not more secure it's just harder to parse (IMO).

[vszakats]

I agree with these being unfamiliar and also not a silver bullet for more secure code,
but I still regard this is a step towards code that is not just secure, but looks secure.
Getting familiar with them is probably also something that may be useful.
Them being used in a small, self-contained, and already unique part of the curl source
code, I think replacing these non-recommended exceptions is useful. Would it be
changing these APIs across multiple files or the whole codebase, it'd be a no, or
they'd need to be hidden under an abstraction.

...unless they come with properties that are breaking, and I suspect we may see this
with the [f]open replacements, so I reverted those earlier.

We already silence these warnings, and that's not the issue, but I think it'd be nicer
to make the code not need this hack.

[MarcelRaad]

+1 for using these in code that's Windows-specific anyway. I think the reason we couldn't use them in the past was legacy MinGW.