Invalid use of std::isspace in StringUtils.h

[SRHMorris]

StringUtils.h contains potentially undefined behaviour when checking for whitespace.

OpenColorIO/src/utils/StringUtils.h

Line 109 in c7ad353

const auto it = std::find_if(str.begin(), str.end(), [](char ch) { return !std::isspace(ch); });

and

OpenColorIO/src/utils/StringUtils.h

Line 126 in c7ad353

std::find_if(str.rbegin(), str.rend(), [](char ch) { return !std::isspace(ch); });

Both should be converted to something equivalent to std::isspace(static_cast<unsigned char>(ch)).

From cppreference:

Like all other functions from cctype, the behavior of std::isspace is undefined if the argument's value is neither representable as unsigned char nor equal to EOF. To use these functions safely with plain chars (or signed chars), the argument should first be converted to unsigned char

I'm hitting asserts when loading a specific ICC profile for a virtual display in these functions when compiling on windows with MSVC:

Debug Assertion Failed!

Program: <redacted>
File: minkernel\crts\ucrt\src\appcrt\convert\isctype.cpp
Line: 36

Expression: c >= -1 && c <= 255

For information on how your program can cause an assertion
failure, see the Visual C++ documentation on asserts.

(Press Retry to debug the application)

[remia]

Thanks for reporting @SRHMorris, I wonder, did you enable special flags to trigger this assertion? Would be nice to have this checked in our Windows CI, we have Debug jobs but these don't seem to complain at the moment.

Probably not tested in our unit tests anyway, I tried it on macOS but it doesn't seem to trigger any assertion when feeding it negatives.

[SRHMorris]

@remia OCIO is compiled with the default cmake debug target. I get the same assertion with the following code:

#include <cctype>
int main()
{
    return std::isspace(-2);
   // return std::isspace(static_cast<unsigned char>(-2)); // works
}

To be clear I don't get this with every ICC profile, just a specific one. Regardless it's still undefined behave if the strings being processed contain negatives (and char is signed).

If I ignore the asserts then everything progresses as expected, so it doesn't seem to cause any problems in release builds (but could potentially). But it's a pain to have to deal with in debug.

[remia]

Understood @SRHMorris, thanks for the details. Are you able to submit a PR for this by any chance?

[SRHMorris]

@remia Not currently, so I'd be happy for anyone else to do it

[doug-walker]

@SRHMorris , could you upload an ICC profile that triggers this issue? We have a volunteer to implement a fix but need a way to test it.

[SRHMorris]

@doug-walker I'm not sure of the legality of distributing this specific one unfortunately.

But this issue doesn't really have anything to do with ICC profiles. It should be enough to just test the functions in StringUtils with utf-8 encoded strings? e.g. something like:

// careful, in c++20 u8 is const char8_t*
const char *s = reinterpret_cast<const char*>(u8"\u0080");

// without the cast to unsigned char this would trigger the assert or lead to UB
LeftTrim(s);

[doug-walker]

OK, thanks @SRHMorris. After PR #1888 is merged, please double check that it solves the issue on your end.

[SRHMorris]

Forgot to check when the PR was merged, but I can confirm that it's fixed in v2.3.1.