ImFontGlyphRangesBuilder has multiple memory issues

[NIKE3500]

Version/Branch of Dear ImGui:

Version: 1.71
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: irrelevant
Compiler: irrelevant
Operating System: irrelevant

My Issue/Question:

Hey,
unfortunately the ImFontGlyphRangesBuilder has multiple memory issues, which I will list below. I decided to gather them all in one issue to avoid having multiple issues lying around that should probably be fixed all at once.

1. UsedChars is not correctly initialized
The member variable will be initialized using the following code (imgui.h, line 2002):

UsedChars.resize(0x10000 / sizeof(int)); 
memset(UsedChars.Data, 0, 0x10000 / sizeof(int));

As memset expects the third parameter to be the byte count to be set. This call causes only 1/sizeof(int) (usually 1/4th for 32 bit integers) of the vector to be initialized.

2. UsedChars is WAY oversized
There are 0x10000 possible values for ImWchar and each one would require one bit in the UsedChar vector to determine whether the character should be added to the range.
The int data type has a minimum guaranteed size of 16 bit == 2 byte.
This means that an initialization using UsedChars.resize(0x10000 / 16); should be sufficient for each character to get one bit to be represented.
However, the actual implementation does UsedChars.resize(0x10000 / sizeof(int)); which is at least 8 times that (for 16 bit integers and 8 bit bytes) and overshoots even more for larger sized integer types.
This should probably be fixed by using a fixed sized type for the vector (if c++11 is supported) and/or by using a type with less room for interpretation instead of int for the vector (e.g. int8_t or unsigned char).

3. GetBit / SetBit only uses 5 bits of each entry in UsedChars
For calculating the bit which represents a single character in the UsedChars vector the following code is used (imgui.h, line 2003):

int off = (n >> 5); 
int mask = 1 << (n & 31); 
return (UsedChars[off] & mask) != 0;

According to this code only the last 5 bits of each entry in the UsedChars vector are being used for storing information on characters. The rest is ignored.
If the UsedChars vector would not be oversized (see above) this would probably cause memory access issues for character with high values.

4. BuildRanges does not have proper bound checks
This is the current code for BuildRanges(imgui_draw.cpp, lines 2392 ff):

for (int n = 0; n < 0x10000; n++)
   if (GetBit(n))
   {
        out_ranges->push_back((ImWchar)n);
        while (n < 0x10000 && GetBit(n + 1))
            n++;
        out_ranges->push_back((ImWchar)n);
    }
out_ranges->push_back(0);

Using this implementation the inner while condition is prone to causing a segmentation fault or overflow when n == 0x10000 - 1, because the GetBit call adds 1 to that value.
Note: This was the original issue that caused me to investigate this because before commit c3c2cd1 the UsedChars vector was sized way smaller and the improper initialization of UsedChars (see 1) caused the relevant bit to be set, thus causing an out of bound read and the corresponding assert in ImVector to be triggered.

5. GetBit/SetBit should not have int as the parameter type
The issue here is that int is only guaranteed to be 16 bits and should be used to index all values for ImWchar which is a typedef of unsigned short and thus also at least 16 bits.
However, int is a signed type and thus insufficient for indexing all characters.

Screenshots/Video
Omitted due to lack of visual issues.

Standalone, minimal, complete and verifiable example:
All issues will be present when using ImFontGlyphRangesBuilder. However, they might not lead to observable symptoms

ImVector<ImWchar> out;
ImFontGlyphRangesBuilder builder;
builder.BuildRanges(&out);

[ocornut]

Thank you very much for this detailed report.

I'll go and look into fixing those soon. This is telling that few people have been using ImFontGlyphRangesBuilder (I originally added it for my own use). Will also start adding more tests for it.

Amusingly: issues 1+2 are kind of cancelling each others but I agree it is suboptimal.
I will keep a 32-bit parameter to GetBit/SetBit because as per #2541 it is expected we eventually support codepoint larger than 16-bit.

[ocornut]

• (1) Fixed
• (2) Fixed. Afaik int is 32 bits on all supported platforms and the rest of the codebase assumes so. I have however changed the underlying storage to use ImU32 as 1<<31 is technically undefined for signed int.
• (3) Your statement is incorrect afaik. mask will contains a single set bit covering the whole 32-bits range.
• (4) Fixed.

Let me know if you think anything is wrong!

[NIKE3500]

Thanks for the quick reaction. As far as I can see all points you mentioned should be fixed by your commit. (5) should be irrelevant for now as long as you expect 32 bit integers.