TableGetSortSpecs can return structure with corrupt ImGuiTableSortSpecs::Specs

[SanaaHamel]

Version/Branch of Dear ImGui:

Version: 1.84 WIP (18303)
Branch: master (commit d5828cd)

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_opengl3.cpp + imgui_impl_glfw.cpp
Compiler: clang 10
Operating System: linux (ubuntu)

My Issue/Question:

TableGetSortSpecs can return structure with a corrupt ImGuiTableSortSpecs::Specs if table sort order hasn't been dirtied.
The contents of the array referenced by ImGuiTableSortSpecs::Specs might also be well formed (but invalid for the table) or point to pure junk.

This matters if you want to know the sort spec for a table even if the spec hasn't changed.
e.g. Your backing store occasionally invalidates and you need to reapply sorting.

Cause is described below. In this context an 'ephemeral' alloc is a mem alloc that is only valid for the duration of a BeginTableEx/EndTable span.

bool    ImGui::BeginTableEx(...)
{
    ... // snip - irrelevant

    // !! Ephemeral storage allocated in `TempData`
    ImGuiTableTempData* temp_data = table->TempData = &g.TablesTempDataStack[g.CurrentTableStackIdx];
    temp_data->TableIndex = table_idx;

    ... // snip - irrelevant
}

ImGuiTableSortSpecs* ImGui::TableGetSortSpecs()
{
... // snip - doesn't matter

    if (table->IsSortSpecsDirty)
        TableSortSpecsBuild(table);  // <--- SortSpecs BUT ONLY IF DIRTY

    // `ImGuiTableSortSpecs::Specs` points to invalid ephemeral alloc from previous begin/end for this table
    return &table->SortSpecs; 
}

void ImGui::TableSortSpecsBuild(ImGuiTable* table)
{
    IM_ASSERT(table->IsSortSpecsDirty);
    TableSortSpecsSanitize(table);

    //  `Specs` points to ephemeral memory
    ImGuiTableTempData* temp_data = table->TempData; 
    temp_data->SortSpecsMulti.resize(table->SortSpecsCount <= 1 ? 0 : table->SortSpecsCount);
    ImGuiTableColumnSortSpecs* sort_specs =
      (table->SortSpecsCount == 0) ? NULL : 
      (table->SortSpecsCount == 1) ? &temp_data->SortSpecsSingle :
                                                             temp_data->SortSpecsMulti.Data;
    ... // snip - doesn't matter
        
    table->SortSpecs.Specs = sort_specs; // <-- POINTS TO EPHEMERAL ALLOC
    table->SortSpecs.SpecsCount = table->SortSpecsCount;
    table->SortSpecs.SpecsDirty = true; // Mark as dirty for user
    table->IsSortSpecsDirty = false; // Mark as not dirty for us
}

Header/docs states the following invariants for TableGetSortSpecs:

// - Call TableGetSortSpecs() to retrieve latest sort specs for the table. NULL when not sorting.
// - When 'SpecsDirty == true' you should sort your data.
// - Lifetime: don't hold on this pointer over multiple frames or past any subsequent call to BeginTable().

Docs for ImGuiTableSortSpecs don't impose any additional invariants.

Screenshots/Video

N/A. Not a layout/visual issue.

Example

This demo draws a full-display window containing two sortable tables.
Click on column header B to change the sorting spec of the first table. This will trigger an assertion when drawing the second table, demonstrating that its sort spec has been (visibly) corrupted.

This sample could trigger an immediate crash if Specs were allocated using a mechanism that doesn't reuse previous allocations.
On my system/run the Specs member points to the same address for both tables. (Legal, but impl doesn't update the array for the second table.)

Source Code:

using namespace ImGui;
auto drawTable = [&](char const* name, std::vector<char const*> columns) {
    if (BeginTable(name, columns.size(), ImGuiTableFlags_Sortable)) {
        for (auto&& col : columns)
            TableSetupColumn(col, ImGuiTableColumnFlags_DefaultSort);
        TableHeadersRow();

        if (auto specs = TableGetSortSpecs()) {
            std::cerr << specs << "\n";
            for (int i = 0; i < specs->SpecsCount; ++i) {
                auto&& info = specs->Specs[i];
                assert(0 <= info.ColumnIndex);
                assert(size_t(info.ColumnIndex) < columns.size());
            }
        }
    }
    EndTable();
};

auto& io = GetIO();
SetNextWindowSize(io.DisplaySize, ImGuiCond_Always);
SetNextWindowPos({0, 0}, ImGuiCond_Always, {0, 0});
if (Begin("__background__", {})) {
    std::cerr << "drawing tables...\n";
    drawTable("###table_col_2", {"A", "B"});
    drawTable("###table_col_1", {"C"});
}
End();

Full about dump:

Dear ImGui 1.84 WIP (18303)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=201703
define: __linux__
define: __GNUC__=4
define: __clang_version__=10.0.1 
--------------------------------
io.BackendPlatformName: imgui_impl_glfw
io.BackendRendererName: imgui_impl_opengl3
io.ConfigFlags: 0x00000001
 NavEnableKeyboard
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x0000000E
 HasMouseCursors
 HasSetMousePos
 RendererHasVtxOffset
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,2048
io.DisplaySize: 1848.00,1016.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 5.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

[ocornut]

Thank you for the report! Looks like this was broken by 4ce6bd8 (May 7)
Will work on a fix soon.

[ocornut]

I can confirm the bug however note that it would only affect the case of not checking the SpecsDirty flag which you are supposed to check before sorting else you will sort too many times.
(Either way it is definitively a bug, just pointing that out)

[ocornut]

Pushed a fix 7c44d06 ! thank you!
Note that bug existed even before 4ce6bd8. Would only materialize if user called TableGetSortSpecs and used data without checking SpecsDirty.