MPD icon indicating copy to clipboard operation
MPD copied to clipboard

system/Error: manually prepare localized error code messages through error category method on windows

Open ibmibmibm opened this issue 6 months ago • 4 comments

FormatMessageA in VFmtLastError and std::system_category::message produce non-utf8 multi-byte string, which causes std::runtime_error("invalid utf8") thrown in fmt::detail::utf8_to_utf16::utf8_to_utf16.

Use FormatMessageW with a custom category to override message method for utf-8 error message.

Remove FormatMessageA in VFmtLastError to remove duplicated error message from error code.

fix #2302

ibmibmibm avatar May 21 '25 03:05 ibmibmibm

We still don't know where the exception came from and your commit message doesn't explain anything. Please add more text to the commit message explaning the whole problem and how your change solves it.

MaxKellermann avatar May 21 '25 05:05 MaxKellermann

Also note the GH Actions failure. The exception message looks suspicious (two trailing newlines?)

MaxKellermann avatar May 21 '25 05:05 MaxKellermann

This PR does at least two distinct things in one commit:

  1. switch from FormatMessageA() to FormatMessageW()
  2. move the FormatMessage call to a new error category

Mixing both in one commit is confusing. The actual bug is only fixed by (1) and (2) has nothing to do with this. The reasons for (2) are not explained anywhere, and the implementation isn't good because it adds massive header dependencies, because you decided to have all of the code in the header for no apparent reason.

For (1) there should be a code comment explaining why you're going through all these hoops. This is all but obvious.

It would also be interesting to know what kind of illegal UTF-8 is generated by FormatMessageA(). I was thinking that FormatMessageA() generates just English ASCII, doesn't it?

MaxKellermann avatar May 23 '25 10:05 MaxKellermann

This PR does at least two distinct things in one commit:

1. switch from FormatMessageA() to FormatMessageW()

2. move the FormatMessage call to a new error category

Mixing both in one commit is confusing. The actual bug is only fixed by (1) and (2) has nothing to do with this. The reasons for (2) are not explained anywhere, and the implementation isn't good because it adds massive header dependencies, because you decided to have all of the code in the header for no apparent reason.

The reason that we need two steps to avoid non-utf8 strings is that VFmtLastError appends non-utf8 message, and std::system_category appends same non-utf8 message again.

I'll split the commit into two.

For (1) there should be a code comment explaining why you're going through all these hoops. This is all but obvious.

It would also be interesting to know what kind of illegal UTF-8 is generated by FormatMessageA(). I was thinking that FormatMessageA() generates just English ASCII, doesn't it?

All A-ending function encode multi-byte string in local codepage, which is CP_ACP in WideCharToMultiByte For example, typical windows machine in japan uses codepage-932 aka shift_jis, and the error message will be "\x8e\x77\x92\xe8\x82\xb3\x82\xea\x82\xbd\x83\x74\x83\x40\x83\x43\x83\x8b\x82\xaa\x8c\xa9\x82\xc2\x82\xa9\x82\xe8\x\82\xdc\x82\xb9\x82\xf1\x81\x42\x0d\x0a" which includes two new line characters at end. Use FORMAT_MESSAGE_MAX_WIDTH_MASK flags will replace that newline character into space character.

ibmibmibm avatar May 23 '25 20:05 ibmibmibm

So it looks like the whole complicated LocaleSafeSystemCategory stuff is not necessary at all, and all we need to do is remove the FormatMessageA() call from VFmtLastError(). This PR does that, but in a very complicated way. I pushed a simpler version to v0.24.x: cd2a58d607e9837477910d8cec0d12ee2db1ad8d

MaxKellermann avatar Jul 31 '25 19:07 MaxKellermann