[gl] avoid crash with system libftgl

[ferdymercury]

Fixes #22076

(Maybe) since ulrichard/ftgl@84869ec#diff-1462936246cba8b5e9ee1c79a1207cea8efb0658be58cd25e7d3acd817ac0ac0R374

so 2.1.3, there is a regression that does not correctly find symbols above code 191. So clamp the text to that max to avoid crashes, if we are using system-ftgl which is for most distributions 2.4 and for some older ones 2.1.3.

Do not clamp for builtin-ftgl since it was forked at 2.1.2, before the regression happened

More details in the linked issue.

fyi @osschar

All latex*.C scripts now run without crashing!

[github-actions]

Test Results

    22 files      22 suites   3d 10h 45m 59s ⏱️
 3 850 tests  3 849 ✅ 0 💤 1 ❌
76 948 runs  76 947 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 49c6c9b.

[linev]

PR goes in right direction, but there should be a way to use all symbols with external ftgl.
We knew that integral symbol fails. So we need to understand which glyph is used with built-in ftgl version.
And then try to find symbol which will give us same glyph with external ftgl.

And most probably such re-coding will be simple - like symb += 0x800.

[ferdymercury]

And most probably such re-coding will be simple - like symb += 0x800.

What do you think about ulrichard/ftgl@84869ec#diff-1462936246cba8b5e9ee1c79a1207cea8efb0658be58cd25e7d3acd817ac0ac0R374

Isn't the fact that it assumes UTF8 multibytes preventing us from doing such simple remapping that you propose?
The missing symbols (int epsilon etc) are exactly the ones they define as multibytes in their table. So I do not know how to fix that without modifying FTGL itself?

[linev]

I am off until Monday, but then I will try to investigate this.
As I mentioned - I still did not understand difference between builtin and external ftgl.

[ferdymercury]

I still did not understand difference between builtin and external ftgl

Builtin (2.1.2) converts each char to one glyph in order.

External (2.1.3) converts each char to one glyph in order if char <192. If char between 192 and 192+16, then it reads also the next char at once and then does the conversion. If 192+32 it's three bytes it assumes.

That's why, when I just prevented nullptr access, I was seeing Int being converted to three random glyphs. So each char int, nabla or epsilon was getting two or three glyphs together, not just one. Probably accessing garbage memory.

[linev]

External (2.1.3) converts each char to one glyph in order if char <192. If char between 192 and 192+16, then it reads also the next char at once and then does the conversion. If 192+32 it's three bytes it assumes.

But then solution is clear - one need to transform string so that final result lead to same set of glyph.

[ferdymercury]

one need to transform string so that final result lead to same set of glyph.

That proposal sounds good, but does not work. Let me explain with the simple example of the Nabla symbol, which is code 209 in symbols.ttf

With external ftgl, since 209 it's above code 191, it thinks it's the start of an UTF8 code, so it reads two more bytes (garbage), so we get three random glyphs drawn.

So you would propose to change the string so that it thinks it's a proper UTF8 code. I can do that using:

         TString utxt;
         const auto len = strlen(txt);
         for(auto i = 0UL; i < len; ++i) {
            if (static_cast<unsigned char>(txt[i]) == static_cast<unsigned char>(209)) { // Nabla is 209 in symbols.ttf and 0xE2 0x88 0x87 in UTF8
               utxt.Append(0xe2); //'\277');
               utxt.Append(0x88);
               utxt.Append(0x87);
            } else if .... else if ... else {
               utxt.Append(txt[i]);
            }
         }
         txt = utxt.Data();

Now, FTGL correctly parses the sequence of three bytes (chars). I debugged and it's assigning this three-byte sequence to an integer 8711. And this integer matches the number that HTML uses to encode Nabla, so great.

So far so good. Now the problem. Then it goes into this function:

unsigned int FTCharmap::FontIndex(const unsigned int characterCode)
{
    return FT_Get_Char_Index(ftFace, characterCode);
}

characterCode is 8711, but this is now a Freetype Library function, no longer FTGL. Since symbols.ttf only defines codes until 256, it does not find characterCode 8711, so he returns the Char Index 0. And then draws 0 (just a bounding box).

In other words, by looking whether a char is or not UTF8, FTGL is blinding itself to the region 192 to 256 of whatever ttf font we have. Remapping won't help.

Of course, one could say: let's extend symbols.ttf so that there is a duplicate glyph at characterCode 8711 and then it would find it. But that's overkill in my opinion.

To me it would make more sense to create FTGL 2.4.1, fix this cumbersome UTF8 identification, pass instead an argument saying whether input is UTF8 or not, and force users to use the builtin version. Then backport to ftgl fork and signal Debian. Otherwise the remapping is very ugly.
For users having older packages, we can fix the nullptr access as done here and print a warning if needed.
Remapping+Extending symbols.ttf looks to me much more complicated than adding an additional FTGL function parameter that avoids the UTF8 detection.

[ferdymercury]

As expected, just changing this in external libftgl:

makes all issues go away:

So I vote for fixing this via a boolean switch argument in FTGL 2.4.1

and allowing buggy versions (>2.1.2 && <2.4.1) with static one-time warning, and no crash by clamping to 191.

[linev]

To me it would make more sense to create FTGL 2.4.1

That you propose is patch external library and convince all major distributions take this patched version. Really?
Looks like changes in libftgl were done for purpose - to provide support of utf-8 coding.
And your proposal fully destroy this functionality.

If external libftgl correctly support utf-8 - one should try to convert our plain C characters string to utf-8 string.
For instance code 200 in utf-8 represented by two bytes: 0xC3 0x88. May be we should try this?

[ferdymercury]

convince all major distributions take this patched version.

FTGL is looking for new mantainer

And your proposal fully destroy this functionality

Noo, i was just proposing to have two different functions, ::Render and ::RenderASCII and the user chooses what conversion to apply by calling one or the other function. ROOT would call the ASCII variant to bypass the UTF8 identification.

For instance code 200 in utf-8 represented by two bytes: 0xC3 0x88. May be we should try this?

Ok, I've tried this, it almost works. There seem to be some residual issues with memory, with only three symbols.

[linev]

But last solution is much-much better!

We do not need to modify ftgl and then maintain it.
I hope one can identify these missing symbols - but even without it will be better than segfaul.

Can one delegate version check to cmake?
It has several functions for that.
And cmake can provide special define which will enable utf-8 conversion

[ferdymercury]

Can one delegate version check to cmake?
It has several functions for that.
And cmake can provide special define which will enable utf-8 conversion

Do you mean sth like:

include(CheckCXXSourceRuns)
set(CMAKE_REQUIRED_INCLUDES ${FTGL_INCLUDE_DIR})
check_cxx_source_runs([[
  #include <cstdlib>
  #include <string>
  #include <sstream>
  #include <array>
  #include <FTGL/ftgl.h>

  int main()
  {
    std::string packageVersion { FTGL::GetString(FTGL::CONFIG_VERSION)};
    std::stringstream ss(packageVersion);
    string sub;
    const char del = '.';
    std::array<int, 3> FTGL_VERSION;
    for(size_t i = 0; i < 3; ++i) {
      getline(ss, sub, del);
      version[i] = atoi(sub);
    }
    const bool usesUTF8 = FTGL_VERSION[0] > 2 || (FTGL_VERSION[0] == 2 && FTGL_VERSION[1] > 1) || (FTGL_VERSION[0] == 2 && FTGL_VERSION[1] == 1 && FTGL_VERSION[1] > 2);
    return usesUTF8 ? 0 : -1;
  }
]] HAVE_UTF8)

if(${HAVE_UTF8})
  target_compile_definitions(RGL PRIVATE HAVE_UTF8)
endif()

[linev]

Do you mean sth like

No, I think that find_package also return version string.
And one can use compare operators like VERSION_EQUAL or VERSION_GREATER or several others.
In my mind, it is more straight-forward than using string version parsing in c++

[ferdymercury]

Unfortunately FindFTGL.cmake does not return VERSION variables, which is why I had to do the CXX workaround