Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] crash if no default fonts are found #4248

Closed
alanb-sony opened this issue Apr 29, 2024 · 4 comments · Fixed by #4249
Closed

[BUG] crash if no default fonts are found #4248

alanb-sony opened this issue Apr 29, 2024 · 4 comments · Fixed by #4249

Comments

@alanb-sony
Copy link

Describe the bug

calling ImageBufAlgo::render_text with font set to "" crashes if none of the default fonts are found. This is because default_font_name ends with a null pointer.

font_file_map.find(fontname) then becomes font_file_map.find(nullptr) which causes std::string{nullptr} which crashes.

I think the nullptr on the end of the array is superfluous and can simply be removed?

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2024

You are quite right!

We used to use a regular 'for' and the nullptr signalled the end of valid names, but at some point we switched to range-for and the nullptr should have been removed when we did that.

I'm preparing a patch as we speak.

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2024

Proposed fix: #4249

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2024

Thanks very much for the full diagnosis, @alanb-sony! It was immediately obvious from your description what needed to be fixed, and I was easily able to go from reading your issue to having a PR submitted in less than 5 minutes. If I'd needed to carefully recreate the conditions of it unable to find any fonts in order to observe the crash and figure out why exactly that was crashing, it might easily have been one of those things I postpone, then falls off the visible part of my inbox, then just sits for months until I notice it again.

Wednesday is our scheduled monthly patch release, so this will surly be incorporated into that release.

lgritz added a commit that referenced this issue Apr 30, 2024
The nullptr at the end of default_font_name was left over from a time
that we iterated by going through array elements until we hit the
nullptr that signalled we had passed the last valid element. At some
point we switched to a range-for, which iterates over every element of
the array without needing an "end sentinel", and we should have
eliminated the nullptr but failed to do so.

Fixes #4248

Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Apr 30, 2024
…tion#4249)

The nullptr at the end of default_font_name was left over from a time
that we iterated by going through array elements until we hit the
nullptr that signalled we had passed the last valid element. At some
point we switched to a range-for, which iterates over every element of
the array without needing an "end sentinel", and we should have
eliminated the nullptr but failed to do so.

Fixes AcademySoftwareFoundation#4248

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@JGoldstone
Copy link
Contributor

Wednesday is our scheduled monthly patch release, so this will surly be incorporated into that release.

Actually you sounded quite appreciative, not surly at all.

lgritz added a commit to lgritz/OpenImageIO that referenced this issue May 16, 2024
…tion#4249)

The nullptr at the end of default_font_name was left over from a time
that we iterated by going through array elements until we hit the
nullptr that signalled we had passed the last valid element. At some
point we switched to a range-for, which iterates over every element of
the array without needing an "end sentinel", and we should have
eliminated the nullptr but failed to do so.

Fixes AcademySoftwareFoundation#4248

Signed-off-by: Larry Gritz <lg@larrygritz.com>
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 a pull request may close this issue.

3 participants