Skip to content

Commit

Permalink
wip
Browse files Browse the repository at this point in the history
  • Loading branch information
@Swiftb0y
Swiftb0y committed Feb 28, 2023
1 parent ea526e2 commit b0004db
Showing 1 changed file with 14 additions and 20 deletions.
34 changes: 14 additions & 20 deletions src/util/string.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,32 +71,26 @@ inline std::size_t wcsnlen_s(
#endif
}

/// Convert a wide-character C string to QString.
/// @brief Convert a wide-character C string to QString.
///
/// 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.
///
/// See also: QString::fromWCharArray()
/// @param wcs
/// @param maxLen maximum numbers of characters
/// @return
inline QString convertWCStringToQString(
const wchar_t* wcs,
std::size_t len) {

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Feb 28, 2023

maxLen
if (!wcs) {
DEBUG_ASSERT(len == 0);
return QString();
}
DEBUG_ASSERT(wcsnlen_s(wcs, len) == len);
const auto ilen = static_cast<int>(len);
DEBUG_ASSERT(ilen >= 0); // unsigned -> signed
switch (sizeof(wchar_t)) {
case sizeof(char16_t):
return QString::fromUtf16(reinterpret_cast<const char16_t*>(wcs), ilen);
case sizeof(char32_t):
return QString::fromUcs4(reinterpret_cast<const char32_t*>(wcs), ilen);
default:
DEBUG_ASSERT(!"unsupported character type");
if (wcs == nullptr) {
return QString();
}
const std::size_t wstr_len = wcsnlen_s(wcs, len);
const auto iLen = static_cast<int>(wstr_len);

// ensure the string is not truncated
DEBUG_ASSERT(wstr_len != len);

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Feb 28, 2023

This condition does not work, because "len" in the standard nomenclature is always a string without the terminator. So it is perfectly valid if "wstr_len == len". I suggest to remove the assertion.

This comment has been minimized.

Sign in to view

Copy link
@Swiftb0y

Swiftb0y Feb 28, 2023

Author Owner

yes, sorry, I meant maxLen. So if wstr_len == maxLen, then according to the doc of wcsnlen_s no null-terminator was found in the string, meaning it got truncated. Does that make sense?

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Feb 28, 2023

I read it like that
"Hello" has len = 5 and if you call it with maxLen = 5 the whole string is used without truncation.
We may verify if there is a terminator after "o" but that read can be an out of bounds access.

So my conclusion is that we don't have an issue here that can happen.

The trunkation has already happened before.

Without that assertion the old way of calling the function will still work.

This comment has been minimized.

Sign in to view

Copy link
@Swiftb0y

Swiftb0y Feb 28, 2023 •
edited
Loading

Author Owner

My interpretation was based on the fact that maxLen contains the length of the buffer the string is in. Not the length of the string being passed. Also maxLen should not be used for manual truncation! Assuming we have buffer that is 10 * sizeof(wchar_t) and that buffer contains "Hello" (with a null terminator). Then wcslen_s will return 5 and everything is good, the terminator is still there and thus its a valid string. Now if that same buffer contained "HelloWorld", the null terminator would have been lost since it doesn't fit into that buffer. The resulting buffer does not contain a valid Cstring and functions that rely on a terminator will read past the buffer. The "HelloWorld" case would trigger the assert and warn us that the contained string is actually invalid.

I acknowledge that this assert is not super useful and only creates a warning thats a symptom of a problem somewhere else. If we remove it, which is fine, we can remove the function even further since QString::fromWCharArray will call it (or something like it) internally anyways.

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Feb 28, 2023

The difference is that QString::fromWCharArray will read behind the buffer if the null terminator is missing. In this case a similar assertion to verify it IS useful. We use here wcsnlen_s which does not suffer this issue.
So there is no assertion required/possible.

This comment has been minimized.

Sign in to view

Copy link
@Swiftb0y

Swiftb0y Mar 1, 2023

Author Owner

The difference is that QString::fromWCharArray will read behind the buffer if the null terminator is missing.

why would it read past the buffer when its given a size that is within the bounds of the buffer? My understanding of QString::fromWCharArray is that it will either read the string until the first null terminator or until the end of buffer (or rather until its specified length). So its just like wcsnlen_s. So yes, the assertion is not needed really, but neither is our use of wcslen_s.

Moreover, looking at the implementations or QString:fromUtf16 and QString::fromUcs4, we see that passing a nullptr as the string is handled by returning an default QString. So in practice, our implementation of convertWCStringToQString is completely redundant and can fully be replaced by direct calls to QString::fromWCharArray!

This comment has been minimized.

Sign in to view

Copy link
@Swiftb0y

Swiftb0y Mar 1, 2023

Author Owner

My understanding of QString::fromWCharArray is that it will either read the string until the first null terminator or until the end of buffer (or rather until its specified length).

Reading more deeply into the docs, I'm not so sure about the fact that it will just run until the first null-byte even when given a larger length. In that case something like this would be appropriate IMO:

QString convertWCStringToQString(
        const wchar_t* wcs,
        std::size_t maxLen) {
    return QString::fromWCharArray(wcs, static_cast<int>(wcsnlen_s(wcs, maxLen));
}

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Mar 1, 2023

Yes, this works, wcsnlen_s safes us from reading past the end and fromCharArray deals with the null pointer.

This comment has been minimized.

Sign in to view

Copy link
@Swiftb0y

Swiftb0y Mar 1, 2023

Author Owner

Great, I'll open an alternative to mixxxdj#11310
// assert no underflow occurred from size_t to int cast
DEBUG_ASSERT(iLen >= 0);

return QString::fromWCharArray(wcs, iLen);
}

} // namespace mixxx
Expand Down

0 comments on commit b0004db

Please sign in to comment.