Closed SRHMorris closed 9 months ago
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.
@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.
Understood @SRHMorris, thanks for the details. Are you able to submit a PR for this by any chance?
@remia Not currently, so I'd be happy for anyone else to do it
@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.
@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);
OK, thanks @SRHMorris. After PR #1888 is merged, please double check that it solves the issue on your end.
Forgot to check when the PR was merged, but I can confirm that it's fixed in v2.3.1.
StringUtils.h contains potentially undefined behaviour when checking for whitespace.
https://github.com/AcademySoftwareFoundation/OpenColorIO/blob/c7ad35340af721c6c3c4780c3a3666b8ce326ba2/src/utils/StringUtils.h#L109
and
https://github.com/AcademySoftwareFoundation/OpenColorIO/blob/c7ad35340af721c6c3c4780c3a3666b8ce326ba2/src/utils/StringUtils.h#L126
Both should be converted to something equivalent to
std::isspace(static_cast<unsigned char>(ch))
.From cppreference:
I'm hitting asserts when loading a specific ICC profile for a virtual display in these functions when compiling on windows with MSVC: