Checking return values of FontEngine::setFont()

[the-shank]

FontEngine::setFont() returns a boolean to indicate if the font was set successfully or there was some issue.

In some parts of the Font.cpp file, the return value of FontEngine::setFont() is not checked. To list a couple:

• https://github.com/mgieseki/dvisvgm/blob/8d4564d1645fbf478c7b542a83d1371f61101f37/src/Font.cpp#L197
• https://github.com/mgieseki/dvisvgm/blob/8d4564d1645fbf478c7b542a83d1371f61101f37/src/Font.cpp#L239

Would you be willing to accept a PR adding checks for these?

[mgieseki]

Did you stumble over issues related to the missing checks? These checks shouldn't really be necessary because the font objects are created by the FontManager, a kind of factory which ensures that the associated fonts exist, i.e it should prevent the creation of invalid fonts. So, setFont should always succeed for these font objects. While it probably doesn't hurt to add the checks, they might slow down the conversion process a bit because the corresponding functions get called a lot. But that's just a guess.

[the-shank]

Sorry for the delay in getting back to you. I missed the part about FontManager.

This particular issue was identified up as a part of a research project we did where in we injected random faults into various libraries (in this case libfreetype) and checked if the applications using these libraries handled these faulty conditions or not.

But if the code already checks that this particular issue is not possible, I guess this one can be ignored.

[the-shank]

However please consider the following case:

1. FontEnginer::setFont() calls libfreetype's FT_New_Face() here
2. In libfreetype, FT_New_Face() calls ft_open_face_internal() here
3. ft_open_face_internal() calls open_face() here, which fails due to failed allocation (somewhere internally in FT_ALLOC() here
4. In this case ft_open_face_internal() would return a non-zero value to signal an error
5. And then FontEngine::setFont() would return a false from here

The possibility of a failed allocation (which lies beyond the control of the application) makes me believe that it might make the code more robust to have these checks added to the application. Please let me know your thoughts.

[mgieseki]

Thank you for the additional information and sorry for the late reply.

It actually shouldn't hurt if setFont fails. dvisvgm prints a warning message in these cases and the resulting SVG will probably look wrong due to missing data. The converter itself is still in a valid state. However, there are a couple of checks missing that prevent dereferencing FontEngine::_currentFace in case of nullptr. I've already added them locally. If I overlooked a more severe aspect here, please let me know.