Potential Bug in ImFont::RenderText, ImFont::CalcTextSizeA #896

New issue

Closed

opened 2016-11-06 04:04:48 +00:00 by burningreggae · 1 comment

burningreggae commented

2016-11-06 04:04:48 +00:00

(Migrated from github.com)

    // Decode and advance source
    const char* prev_s = s;
    unsigned int c = (unsigned int)*s;
    if (c < 0x80)
    {
        s += 1;
    }
    else
    {
        s += ImTextCharFromUtf8(&c, s, text_end);
        if (c == 0)
            break;
    }

I'm pretty sure that if ( c == 0 ) should be outside the else path..

    if (c < 0x80) s += 1;
    else s += ImTextCharFromUtf8(&c, s, text_end);
    if (c == 0) break;

// Decode and advance source const char* prev_s = s; unsigned int c = (unsigned int)*s; if (c < 0x80) { s += 1; } else { s += ImTextCharFromUtf8(&c, s, text_end); if (c == 0) break; } I'm pretty sure that if ( c == 0 ) should be outside the else path.. if (c < 0x80) s += 1; else s += ImTextCharFromUtf8(&c, s, text_end); if (c == 0) break;

ocornut commented

2016-11-06 14:26:05 +00:00

(Migrated from github.com)

It probably should be clarified in the comment, but unexpectedly this is actually correct.

We already have a text_end pointer (which typically point to the end of string \0) so for single-byte character we don't need this test because a \0 would never happen or technically would be legal (not but not super useful) if we pass it a range that includes \0.

The test here handles malformed UTF-8.

(We receive a non NULL text_end pointer earlier in the stack in the majority of cases, so even the first two lines of the functions which computes text_end is a bit of a unlikely fallback for cases when users calls RenderText() themselves,. This is why it isn't worth optimizing for single-pass zero-check here.. Actually adding the extra if() would likely incur extra minor cost)

It probably should be clarified in the comment, but unexpectedly this is actually correct. We already have a `text_end` pointer (which typically point to the end of string \0) so for single-byte character we don't need this test because a \0 would never happen _or_ technically would be legal (not but not super useful) if we pass it a range that includes \0. The test here handles malformed UTF-8. (We receive a non NULL `text_end` pointer earlier in the stack in the majority of cases, so even the first two lines of the functions which computes `text_end` is a bit of a unlikely fallback for cases when users calls RenderText() themselves,. This is why it isn't worth optimizing for single-pass zero-check here.. Actually adding the extra if() would likely incur extra minor cost)