winapi: simplify curlx_get_winapi_error() function

[dEajL3kA]

Use FormatMessageA() to get the error message as multibyte-character string (local codepage) right away, instead of using FormatMessageW() and then converting from Unicode (UTF-16) string to multibyte-character string (local codepage) manually via wcstombs() function.

Ref: a326877 #6065

[vszakats]

Would FormatMessageA() return a failure if the passed buflen exceeds 64K?

[dEajL3kA]

Microsoft says:

The output buffer cannot be larger than 64K bytes.

Wine does this:

    if (size >= 32768)
    {
        SetLastError( ERROR_INVALID_PARAMETER );
        return 0;
    }

Maybe we should check for 32768 then? 🤔

[dEajL3kA]

On Windows 11, size greater than 0xFFFF fails with ERROR_INSUFFICIENT_BUFFER.

Don't know if limit maybe was 0x7FFF on older versions ?!

[vszakats]

How about doing it like before, omit the the manual check, and let the API return an error as necessary? it sees the size and can (and does) act upon it.

[dEajL3kA]

What about limiting the size to 0x7FFF?

If caller offers a bigger buffer, we can simply pretend the buffer is only 0x7FFF in size, so that the API won't fail.

[dEajL3kA]

In the "old" version, buflen was not passed to FormatMessage(), since a local wide-char buffer was used.

Now that we pass buf and buflen to FormatMessage() directly, we have to cast buflen from size_t to DWORD.

This creates a possible overflow that did not exist before.

To avoid overflow, we need to bound buflen to MAXDWORD. But if we do that, we could simply bound to 0x7FFF, because that not only avoids the possible overflow but also fixes the possible API failure at the same time.

Not bounding it at all, just hoping caller will never pass a value that would overflow, is precarious, in my opinion.

---

Goal of this PR is to get rid of an unnecessary character set conversion.

[vszakats]

Where is the overflow, if passing 256 bytes max throughout the codebase?
Possible API failures are handled by checking the return value.

Why not keep it simple?

[bagder]

We could potentially add an assert for it to catch ourselves if we ever change the usage in a future.

[dEajL3kA]

Why not keep it simple?

curlx_get_winapi_error() is is used in curlx_strerr(), which passes through the buflen parameter. And curlx_strerr() is used in many places. On a quick search, it appears that all curlx_strerr() invocations are "internal" calls with a fixed-size buffer. But this is not trivial to verify. And even if it is true now, it could change in the future. So I'd say, better safe than sorry. After all, clamping buflen to the range that FormatMessage() actually supports does not add that much of "complexity". Especially considered that we saved the entire string conversion.

[vszakats]

I still think that adding magic numbers like 0xFFFF, 0x7FFF and duplicating the API's work is completely overkill.
We're checking for the API result and fail if it fails. It's known to fail if the passed size is too large.

casting size_t to DWORD is done elsewhere in the code and will truncate the value to DWORD_MAX, and make the function fail.

curlx_strerr() all follow the same pattern throughout the codebase, and even a call accidentally passed a 1GB or 4GB buffer to it, FormatMessage() errors out.

[dEajL3kA]

Added DEBUGASSERT, as suggested.

[dEajL3kA]

Why not keep it simple?

curlx_get_winapi_error() is is used in curlx_strerr(), which passes through the buflen parameter. And curlx_strerr() is used in many places. On a quick search, it appears that all curlx_strerr() invocations are "internal" calls with a fixed-size buffer. But this is not trivial to verify. And even if it is true now, it could change in the future. So I'd say, better safe than sorry. After all, clamping buflen to the range that FormatMessage() actually supports does not add that much of "complexity". Especially considered that we saved the entire string conversion.

[jay]

This function originally used FormatMessage for Windows CE and FormatMessageA for all other Windows. Some versions of Windows CE did not have FormatMessageA and always defined FormatMessage as FormatMessageW (or maybe just had a FormatMessage with wide characters I don't remember exactly).

Switching to FormatMessageA should be fine since we recently ended Windows CE support.

[dEajL3kA]

Is anything else needed?

[jay]

Is anything else needed?

Looks ok to me. I don't think the misc checks CI failure is related so I set it to run again. I'd get rid of the WINE URL. The assert is not strictly needed but I'm fine with it. What do you think Viktor?

[vszakats]

Is anything else needed?

Looks ok to me. I don't think the misc checks CI failure is related so I set it to run again. I'd get rid of the WINE URL. The assert is not strictly needed but I'm fine with it. What do you think Viktor?

I'd also get rid of the long URL, and remove DEBUGASSERT,
since the API already does this check for us. If we want to keep it
to verify our own code, it'd be more useful IMO to make it check for
STRERROR_LEN or a reasonable range we'd actually use in internal
calls, like > 64 && < 1024, instead of hard-coding an internal limit
of a particular revision of the WINE implementation.

[jay]

Landed without the assert. Thanks