Silent NULL reference when merging into an invalid ImFont.

[JaedanC]

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.

[ocornut]

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();
}

[JaedanC]

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.