Fix convertWCStringToQString assert

[JoergAtGithub]

convertWCStringToQString is only used at a few places in HID code currently:
Either a constant is used with the maximum string length:

mixxx/src/controllers/hid/hidiothread.cpp

Lines 104 to 108 in 1b99e57

	qCWarning(m_logOutput) << "Unable to read buffered HID InputReports from"
	<< m_deviceInfo.formatName() << ":"
	<< mixxx::convertWCStringToQString(
	hid_error(m_pHidDevice),
	kMaxHidErrorMessageSize);

Or it's calculated it's the caller using wcsnlen_s:

mixxx/src/controllers/hid/hiddevice.cpp

Lines 40 to 43 in 1b99e57

	m_manufacturerString(mixxx::convertWCStringToQString(
	device_info.manufacturer_string,
	mixxx::wcsnlen_s(device_info.manufacturer_string,
	kDeviceInfoStringMaxLength))),

In th case of a constant, we always get an assert, in the second case we compare two values calculated by the same wcsnlen_s function. Therefore the actual length check assert is usesless.

This PR changes the code, that we detect strings longer than the specified length. But I wonder, if this assert is useful at all, since nothing breaks if these strings are longer.
Furthermore the function wcsnlen_s which made problems on OpenBCD #11083 is only existing in the Mixxx code, to generate this one DEBUG_ASSERT.

[Swiftb0y]

I spend a bunch of time trying to understand the purpose of the function in the first place. I think we should remove it entirely. The explanation states that "We cannot use Qts wchar_t functions, since they may work or not depending on the '/Zc:wchar_t-' build flag in the Qt configs on Windows build." However, Microsoft docs state: "We do not recommend /Zc:wchar_t- because the C++ standard requires that wchar_t be a built-in type.". So if Qt is really building with that, it is a Qt bug IMO. We should be able to rely on Qt's wchar_t functions. Also, I was not able to find any warning in Qt's doc that mentioned the issues.
Going through the blame, the comment eventually originated from 344ae07

I gather from this comment that we can remove convertWCStringToQString completely anyways since it was only needed for Qt4? 344ae07#r14482186

Maybe @daschuer knows more since he introduced that change.

[daschuer]

This PR fixes an typo that has been introduced here: 3c58f6b#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R43

I can confirm that this is fixed here.
In terms of avoid scope creep, I vote for merging this PR as it is. An postpone future improvements.

[daschuer]

In the Qt 5 code I can find this:

• MSVC desktop builds now use -Zc:wchar_t.
  https://github.com/qt/qtbase/blob/9db7cc79a26ced4997277b5c206ca15949133240/dist/changes-5.0.0#L946
  https://github.com/qt/qtbase/blob/5.12/mkspecs/common/msvc-desktop.conf#L37

We use win32-msvc.conf including msvc-desktop.conf
https://github.com/microsoft/vcpkg/blob/master/ports/qt5-base/cmake/find_qt_mkspec.cmake#L24

Conclusion: We can assume -Zc:wchar_t is set.

[daschuer]

Even without explicit setting the flag the option is on by default:
https://learn.microsoft.com/en-us/cpp/build/reference/zc-wchar-t-wchar-t-is-native-type?view=msvc-140

The /Zc:wchar_t option is on by default in C++ compilations, and is ignored in C compilations. The /permissive- option does not affect /Zc:wchar_t.

And in addition QString::fromWCharArray() is defined in a qstring.h build with the Mixxx compiler flags.

[daschuer]

Conclusion: We may use QString::fromWCharArray() but it is is not null-safe and it uses the not optimized while loop that has been dismissed here: #11083 (comment)

See:
https://github.com/qt/qtbase/blob/dev/src/corelib/text/qstring.h#L1054-L1058
https://github.com/qt/qtbase/blob/5c5440338ff816bbce7b7c2d8b2b4cdd6fe91d92/src/corelib/text/qstring.cpp#L5819

[daschuer]

@Swiftb0y Can we merge this now?

[Swiftb0y]

If we want to merge this, I think this needs more discussion. Currently the primary issue is that the purpose of maxLen is unclear. Is it supposed to be the string length like in QString::fromWCharArray() or is it supposed to be a bound like in wcsnlen_s? We can see that discussion even in the current code as both examples linked in @JoergAtGithub's initial post use it differently.

[Swiftb0y]

We may use QString::fromWCharArray() but it is is not null-safe and it uses the not optimized while loop that has been dismissed here:

The while loop is in QString::fromUcs4 which we call just like QString::fromWCharArray(). There is no difference.

[daschuer]

Oh yes right, my bad. The original code is just right.
The issue is at the usage.

We need to fix this:

mixxx/src/controllers/hid/hidiothread.cpp

Lines 106 to 108 in 7307fc9

	<< mixxx::convertWCStringToQString(
	hid_error(m_pHidDevice),
	kMaxHidErrorMessageSize);

And this:

mixxx/src/controllers/hid/hidiooutputreport.cpp

Lines 127 to 129 in 7307fc9

	<< mixxx::convertWCStringToQString(
	hid_error(pHidDevice),
	kMaxHidErrorMessageSize);

By replacing the constant with
mixxx::wcsnlen_s() instead of the constant.

[daschuer]

maxLen + 1 may lead to an out of bounds access. What is the purpose?

[Swiftb0y]

By replacing the constant with
mixxx::wcsnlen_s() instead of the constant.

I argue the opposite. IMO convertWCStringToQString should be like wcsnlen_s. The len parameter should be the maximum length of the string (or rather the size of the buffer where the string is stored). The contained debug assert then needs to be flipped to warn for the cases where the contained string is truncated and thus the null byte is missing.
This is how I'd propose to refactor convertWCStringToQString and then we'd need to fix the callsites to always take the buffer size (not sure if that should be in bytes or characters): Swiftb0y@b0004db

[daschuer]

I don't mind which way we turn it. One will fix the recently introduced wrong usage of the existing function and the other will change the behaviour of the existing function towards the expectations in the new code.