ImageEngine / cortex

Libraries for visual effects software development
Other
528 stars 123 forks source link

OpenImageIOAlgo : Properly handle empty metadata values #1396

Closed ivanimanishi closed 10 months ago

ivanimanishi commented 10 months ago

ustring::c_str() returns a NULL pointer for empty strings.

https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/master/src/include/OpenImageIO/ustring.h#L139-L143 https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/master/src/include/OpenImageIO/ustring.h#L290

This error was noticed when other OIIO changes started returning new metadata keys for which the values were empty strings, and thus causing segfaults when the NULL pointer wasn't handled properly (RuntimeError: basic_string::_S_construct null not valid).

The solution here is to use string() instead of c_str(), which returns an empty string in those cases, and is expected to be equally performant since the std strings are already stored in the internal ustring table: https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/master/src/include/OpenImageIO/ustring.h#L760

The image that was causing the errors is a large one and I couldn't easily create an alternative one to include as a unittest. For reference though, the image being read was a tiff and the offending metadata key was ICCProfile:platform_signature.

Also, there are other references to ustring::c_str() in this file. Those however are likely expecting basic c-string pointers, so I didn't change them. @johnhaddon, if you think that any of those would also cause issues if they got a NULL pointer back, we probably want to address them too, likely by checking for the NULL value specifically and handling that differently.

Checklist

johnhaddon commented 10 months ago

LGTM. I'd be curious to know OIIO's design rationale for allowing ustring.c_str() to be different to ustring.string().c_str() - seems rather surprising. From what I can see though, the other c_str() calls in this file are guaranteed to be non-null, so hopefully this is the only mitigation required. Thanks Ivan!