Silent NULL reference when merging into an invalid ImFont. #6446

New issue

Closed

opened 2023-05-23 04:09:23 +00:00 by JaedanC · 2 comments

JaedanC commented

2023-05-23 04:09:23 +00:00

(Migrated from github.com)

Version: 1.89.6 WIP (18954)
Branch: docking
Back-ends: imgui_impl_glfw.cpp + imgui_impl_opengl3.cpp

I am currently writing a wrapper on top of dear_bindings so please forgive any c++ syntax errors as I have translated code from C to C++. Silent NULL references are hard to deal with in binding languages :(

It's possible to crash ImGui (with assertions on) using the code below with regards to merging fonts.

The fonts themselves don't matter, just that these conditions are met:

Font A does not use any characters that are in range.
Font B uses a character in range.

I've chosen to use a Math symbol font to show this.

ImGui::ImFontGlyphRangesBuilder builder;
// Add any char that doesn't exist in font A, but does in B.
builder.AddText("Ω"); // 937 is an example
// builder.AddText("Some valid chars Ω");

ImGui::ImVector<ImWchar> c_range;
builder.BuildRanges(&c_range);

ImGui::ImFontAtlas* atlas = ImGui::GetIO()->Fonts;
// atlas->AddFontDefault();

ImGui::ImFontConfig cfg;
atlas->AddFontFromFileTTF("fonts/ProggyClean.ttf", 14.0f, &cfg, &c_range);
// atlas->AddFontFromFileTTF("fonts/ProggyClean.ttf", 14.0f, &cfg, atlas->GetGlyphRangesDefault());
cfg.MergeMode = true;
atlas->AddFontFromFileTTF("fonts/DroidSans.ttf", 20.0f, &cfg, &c_range);

// Crashes in here
atlas->Build();

Excerpt from imgui_draw.cpp ImFontAtlasBuildWithStbTruetype

static bool ImFontAtlasBuildWithStbTruetype(ImFontAtlas* atlas)
// ...
for (int src_i = 0; src_i < src_tmp_array.Size; src_i++)
    {
        ImFontBuildSrcData& src_tmp = src_tmp_array[src_i];
        if (src_tmp.GlyphsCount == 0)
            continue;

        // ...
        ImFontAtlasBuildSetupFont(atlas, dst_font, &cfg, ascent, descent);
        // ...

        for (int glyph_i = 0; glyph_i < src_tmp.GlyphsCount; glyph_i++)
        {
            // ...
            dst_font->AddGlyph(&cfg, (ImWchar)codepoint, q.x0 + font_off_x, q.y0 + font_off_y, q.x1 + font_off_x, q.y1 + font_off_y, q.s0, q.t0, q.s1, q.t1, pc.xadvance);
        }
    }

Inside ImFont::AddGlyph the line with ContainerAtlas->TexGlyphPadding causes the crash since ContainerAtlas is NULL.

ContainerAtlas would normally be set by ImFontAtlasBuildSetupFont() (Shown above) but:

Font A has no glyphs, (ImFontAtlasBuildSetupFont() is never run)
Font B has MergeMode = True (ContainerAtlas is not set inside ImFontAtlasBuildSetupFont()), the resulting font does not have a valid ContainerAtlas, resulting in the NULL access inside AddGlyph().

You can see this by uncommenting builder.AddText("Some valid chars Ω");. Then the crash doesn't happen because Font A is valid. Otherwise trying to use Font A (without a merge) would normally be caught by IM_ASSERT(font && font->IsLoaded()); inside imgui.cpp.

I think I would be good to IM_ASSERT to check if ContainerAtlas is valid, just in case the user tries merging two fonts like this.

My suggestion is to add the following inside ImFont::AddGlyph

// ...
IM_ASSERT(ContainerAtlas != NULL && "Cannot merge a font into an invalid font. Perhaps there are no valid glyphs in the first font?");
float pad = ContainerAtlas->TexGlyphPadding + 0.99f;

Or something similar to indicate to the user that they have made a mistake.

Version: 1.89.6 WIP (18954) Branch: docking Back-ends: imgui_impl_glfw.cpp + imgui_impl_opengl3.cpp I am currently writing a wrapper on top of `dear_bindings` so please forgive any c++ syntax errors as I have translated code from C to C++. Silent NULL references are hard to deal with in binding languages :( --- It's possible to crash ImGui (with assertions on) using the code below with regards to merging fonts. The fonts themselves don't matter, just that these conditions are met: - Font A *does not* use any characters that are in range. - Font B uses a character in range. I've chosen to use a Math symbol font to show this. ```c++ ImGui::ImFontGlyphRangesBuilder builder; // Add any char that doesn't exist in font A, but does in B. builder.AddText("Ω"); // 937 is an example // builder.AddText("Some valid chars Ω"); ImGui::ImVector<ImWchar> c_range; builder.BuildRanges(&c_range); ImGui::ImFontAtlas* atlas = ImGui::GetIO()->Fonts; // atlas->AddFontDefault(); ImGui::ImFontConfig cfg; atlas->AddFontFromFileTTF("fonts/ProggyClean.ttf", 14.0f, &cfg, &c_range); // atlas->AddFontFromFileTTF("fonts/ProggyClean.ttf", 14.0f, &cfg, atlas->GetGlyphRangesDefault()); cfg.MergeMode = true; atlas->AddFontFromFileTTF("fonts/DroidSans.ttf", 20.0f, &cfg, &c_range); // Crashes in here atlas->Build(); ``` Excerpt from `imgui_draw.cpp` `ImFontAtlasBuildWithStbTruetype` ```c++ static bool ImFontAtlasBuildWithStbTruetype(ImFontAtlas* atlas) // ... for (int src_i = 0; src_i < src_tmp_array.Size; src_i++) { ImFontBuildSrcData& src_tmp = src_tmp_array[src_i]; if (src_tmp.GlyphsCount == 0) continue; // ... ImFontAtlasBuildSetupFont(atlas, dst_font, &cfg, ascent, descent); // ... for (int glyph_i = 0; glyph_i < src_tmp.GlyphsCount; glyph_i++) { // ... dst_font->AddGlyph(&cfg, (ImWchar)codepoint, q.x0 + font_off_x, q.y0 + font_off_y, q.x1 + font_off_x, q.y1 + font_off_y, q.s0, q.t0, q.s1, q.t1, pc.xadvance); } } ``` Inside `ImFont::AddGlyph` the line with `ContainerAtlas->TexGlyphPadding` causes the crash since `ContainerAtlas` is NULL. `ContainerAtlas` would normally be set by `ImFontAtlasBuildSetupFont()` (Shown above) but: - Font A has no glyphs, (`ImFontAtlasBuildSetupFont()` is never run) - Font B has MergeMode = True (ContainerAtlas is not set inside `ImFontAtlasBuildSetupFont()`), the resulting font does not have a valid ContainerAtlas, resulting in the NULL access inside `AddGlyph()`. You can see this by uncommenting `builder.AddText("Some valid chars Ω");`. Then the crash doesn't happen because Font A is valid. Otherwise trying to use Font A (without a merge) would normally be caught by `IM_ASSERT(font && font->IsLoaded());` inside `imgui.cpp`. I think I would be good to `IM_ASSERT` to check if `ContainerAtlas` is valid, just in case the user tries merging two fonts like this. My suggestion is to add the following inside `ImFont::AddGlyph` ```c++ // ... IM_ASSERT(ContainerAtlas != NULL && "Cannot merge a font into an invalid font. Perhaps there are no valid glyphs in the first font?"); float pad = ContainerAtlas->TexGlyphPadding + 0.99f; ``` Or something similar to indicate to the user that they have made a mistake.

ocornut commented

2023-05-23 08:58:28 +00:00

(Migrated from github.com)

Thanks for reporting this in details and carefully. I confirmed the edge case and push a fix 08145bc.
Even though it is a little odd, it seems valid and instead of asserting I let it run by removing the (src_tmp.GlyphsCount == 0) check.

I am currently writing a wrapper on top of dear_bindings

Pleased to hear that dear_bindings may be useful to you!

so please forgive any c++ syntax errors as I have translated code from C to C++.

For reference the repro in a version that compiles in C++:

{
    ImFontGlyphRangesBuilder builder;
    // Add any char that doesn't exist in font A, but does in B.
    builder.AddChar(937); // 937 is an example
    //// builder.AddText("Some valid chars");

    ImVector<ImWchar> c_range;
    builder.BuildRanges(&c_range);

    ImFontAtlas* atlas = ImGui::GetIO().Fonts;
    // atlas->AddFontDefault();

    ImFontConfig cfg;
    atlas->AddFontFromFileTTF("../../misc/fonts/ProggyClean.ttf", 14.0f, &cfg, c_range.Data);
    // atlas->AddFontFromFileTTF("fonts/ProggyClean.ttf", 14.0f, &cfg, atlas->GetGlyphRangesDefault());
    cfg.MergeMode = true;
    atlas->AddFontFromFileTTF("../../misc/fonts/DroidSans.ttf", 20.0f, &cfg, c_range.Data);

    // Crashes in here
    atlas->Build();
}

Thanks for reporting this in details and carefully. I confirmed the edge case and push a fix 08145bc. Even though it is a little odd, it seems valid and instead of asserting I let it run by removing the `(src_tmp.GlyphsCount == 0)` check. > I am currently writing a wrapper on top of dear_bindings Pleased to hear that dear_bindings may be useful to you! > so please forgive any c++ syntax errors as I have translated code from C to C++. For reference the repro in a version that compiles in C++: ```cpp { ImFontGlyphRangesBuilder builder; // Add any char that doesn't exist in font A, but does in B. builder.AddChar(937); // 937 is an example //// builder.AddText("Some valid chars"); ImVector<ImWchar> c_range; builder.BuildRanges(&c_range); ImFontAtlas* atlas = ImGui::GetIO().Fonts; // atlas->AddFontDefault(); ImFontConfig cfg; atlas->AddFontFromFileTTF("../../misc/fonts/ProggyClean.ttf", 14.0f, &cfg, c_range.Data); // atlas->AddFontFromFileTTF("fonts/ProggyClean.ttf", 14.0f, &cfg, atlas->GetGlyphRangesDefault()); cfg.MergeMode = true; atlas->AddFontFromFileTTF("../../misc/fonts/DroidSans.ttf", 20.0f, &cfg, c_range.Data); // Crashes in here atlas->Build(); } ```

❤️ 1

JaedanC commented

2023-05-23 09:23:44 +00:00

(Migrated from github.com)

That's great. Solved at the source. I'll make sure to spin up a working c++ example in future.
dear_bindings is S tier.

That's great. Solved at the source. I'll make sure to spin up a working c++ example in future. dear_bindings is S tier.