Skip to content

Conversation

ivanimanishi
Copy link
Member

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

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Cortex project's prevailing coding style and conventions.

`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.

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
@johnhaddon
Copy link
Member

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!

@johnhaddon johnhaddon merged commit f62076e into ImageEngine:RB-10.5 Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants