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

BeginTabItem negative NameOffset - Why was NameOffset changed from int to short? #4176

Closed
0xSombra opened this issue May 26, 2021 · 3 comments
Labels

Comments

@0xSombra
Copy link

Commit introducing this issue: 46d7520
Issue: offset points outside of the buffer (backwards because it's casted to signed short without checks)
Code:

ImGui::SetNextWindowSize(ImVec2(400, 400), ImGuiCond_Once);
if (ImGui::Begin("NameOffset"))
{
    if (ImGui::BeginTabBar("tabbar", ImGuiTabBarFlags_TabListPopupButton))
    {
        // ... or long tab names, total characters should be > 32767
        for (auto i = 0; i != 2048; ++i)
        {
            ImGui::PushID(i);
            if (ImGui::BeginTabItem("ABCDEFGHIJKLMNOPQRSTUVWXYZ"))
                ImGui::EndTabItem();
            ImGui::PopID();
        }
        ImGui::EndTabBar();
    }
    ImGui::End();
}

NameOffsetBug

I don't know why it was changed to S16 but changing it to S32 fixes this as expected
This is how it looks like in the latest commit on master:

imgui/imgui_internal.h

Lines 1956 to 1971 in 04fd507

struct ImGuiTabItem
{
ImGuiID ID;
ImGuiTabItemFlags Flags;
int LastFrameVisible;
int LastFrameSelected; // This allows us to infer an ordered list of the last activated tabs with little maintenance
float Offset; // Position relative to beginning of tab
float Width; // Width currently displayed
float ContentWidth; // Width of label, stored during BeginTabItem() call
ImS16 NameOffset; // When Window==NULL, offset to name within parent ImGuiTabBar::TabsNames
ImS16 BeginOrder; // BeginTabItem() order, used to re-order tabs after toggling ImGuiTabBarFlags_Reorderable
ImS16 IndexDuringLayout; // Index only used during TabBarLayout()
bool WantClose; // Marked as closed by SetTabItemClosed()
ImGuiTabItem() { memset(this, 0, sizeof(*this)); LastFrameVisible = LastFrameSelected = -1; NameOffset = BeginOrder = IndexDuringLayout = -1; }
};

imgui/imgui_widgets.cpp

Lines 7767 to 7769 in 04fd507

// Append name with zero-terminator
tab->NameOffset = (ImS16)tab_bar->TabsNames.size();
tab_bar->TabsNames.append(label, label + strlen(label) + 1);

imgui/imgui_internal.h

Lines 2010 to 2014 in 04fd507

const char* GetTabName(const ImGuiTabItem* tab) const
{
IM_ASSERT(tab->NameOffset != -1 && (int)tab->NameOffset < TabsNames.Buf.Size);
return TabsNames.Buf.Data + tab->NameOffset;
}

@ocornut
Copy link
Owner

ocornut commented May 26, 2021

We will revert and fix this but out of curiosity, what got you into using 32K worth of name data?

@ocornut ocornut added the bug label May 26, 2021
@0xSombra
Copy link
Author

Was trying to test a file parser. loaded all files at once, forgot to not render them and it crashed in BeginTabItem
I wouldn't normally use this amount of tabs, but S16 is very limiting imo

@ocornut
Copy link
Owner

ocornut commented May 27, 2021

Pushed a fix for it now, thank you!

@ocornut ocornut closed this as completed May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants