Font cache is not thread safe when using many different font sizes

[ttenbrinkzenz]

Reproducer in C++17

// CImg draw_text() thread-safety crash reproducer
//
// Root cause: CImg::font() acquires mutex(11), builds a font in a static
// CImgList fonts[16] cache at slot 'ind', releases the mutex, then returns
// a *reference* into fonts[ind]. A concurrent thread can evict that slot via
// memmove+memset while the first thread still holds the reference, causing a
// crash inside get_resize() / _draw_text().
//
// Confirmed on CImg v3.0.3 and v3.7.0 with C++17. Reproducer was tested on Windows
// but originaly this happened in a Linux production environment.

#include <atomic>
#include <csignal>
#include <cstdio>
#include <thread>
#include <vector>

#ifdef _WIN32
#  include <windows.h>
#  include <crtdbg.h>
#endif

#define cimg_display  0
#define cimg_verbosity 0
#include "CImg.h"
using namespace cimg_library;

static void crash_handler(int sig) {
    fprintf(stderr, "\nCRASH: signal %d (%s) -- CImg font() cache race confirmed.\n",
            sig, sig == SIGABRT ? "SIGABRT" : sig == SIGSEGV ? "SIGSEGV" : "?");
    fflush(stderr);
    _exit(1);
}

static std::atomic<bool> go{false};

static void worker(int id) {
    while (!go.load(std::memory_order_acquire)) {}
    for (int i = 0; ; ++i) {
        // 50 distinct sizes forces cache eviction (cache holds only 16 slots)
        unsigned int font_size = 13 + static_cast<unsigned int>(id * 7 + i) % 50;
        CImg<unsigned char> img;
        const unsigned char color = 1;
        img.draw_text(0, 0, "Test", &color, 0, 1.f, font_size);
        if (img.width() < 0) fputc('.', stdout); // prevent elision
    }
}

int main() {
#ifdef _WIN32
    _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
    _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
    _CrtSetReportMode(_CRT_ERROR,  _CRTDBG_MODE_FILE);
    _CrtSetReportFile(_CRT_ERROR,  _CRTDBG_FILE_STDERR);
    SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX | SEM_NOOPENFILEERRORBOX);
#endif
    signal(SIGABRT, crash_handler);
    signal(SIGSEGV, crash_handler);
    cimg::exception_mode(0);

    printf("CImg v%u: 8 threads calling draw_text() concurrently with 50 font sizes.\n"
           "Expected: SIGABRT/SIGSEGV crash within seconds.\n\n", cimg_version);
    fflush(stdout);

    std::vector<std::thread> threads;
    for (int i = 0; i < 8; ++i)
        threads.emplace_back(worker, i);
    go.store(true, std::memory_order_release);
    for (auto& t : threads) t.join();

    printf("No crash.\n");
    return 0;
}

Please let me know if there is some way to avoid this, but our application uses quite a few different font sizes and we'd rather not have a global lock on every draw_text.

Basically, if I understand it correctly, the font cache returns a reference, but this gets invalidated the moment the cache is full and it moves everything left, causing crashes when this one is used.

[dtschump]

Thanks for your report, I will investigate this.

[dtschump]

OK, so after analyzing the code, here is my opinion on this issue:

To make the ‘draw_text()’ function thread-safe when called with a ‘font_height’ parameter, there are only three possible options:

1. Replace the line const CImgList<ucharT>& font = CImgList<ucharT>::font(font_height,true); in the draw_text() fonction (l. 52698), by const CImgList<ucharT> font = CImgList<ucharT>::font(font_height,true); to force font to be an independant copy (not a reference anymore). Main disavantadge is the computational/memory cost of doing a copy of an entire font.
2. Add a lock in all functions calling CImgList<ucharT>::font(font_height,true); to ensure the font cache is never modified during a use of a font reference. The lock in function font() could even be removed in this case, replaced by a more global lock in calling functions. Disavantadge : it breaks the multi-threading properties of the calls to draw_text().
3. Add a note in the documentation saying that CImg<T>::draw_text() is not thread-safe.

At this point, I'm not sure what the best solution is. But if we want to make draw_text() thread-safe, it will come with additionnal costs anyway.

Also, for your specific issue, with the current version of CImg.h: An easy work-around for you would be to use one of the version of draw_text() method that is defined with an argument that is directly const CImgList<t>* const font and not const unsigned int font_height=13, for instance:

    CImg<T>& draw_text(const int x0, const int y0,
                       const char *const text,
                       const tc1 *const foreground_color, const tc2 *const background_color,
                       const float opacity, const CImgList<t>* const font, ...);

where font is a copy of what has been returned by CImg<T>::font() that you stores somewhere in your code.

What do you think?

[ttenbrinkzenz]

Ah, I hadn't noticed the ability to store the fonts ourselves, that's probably what we will move to, thanks a lot for your help! For point 1 I'm not sure how high the cost is, not familiar enough and was not the person who wrote the code using CImg :) I agree point 2 would not be great either to have by default in the library (we temporarily did something like that to avoid the crashes we were having). Point 3 is most likely the best and easiest, be sure to then put your suggestion of using the other font function there as well.

One other idea I would have is to allow increasing the size of the font cache? Or to still use a mutex but to make it still parallelizable when your within capacity of the cache (not sure if that's possible honestly)?

[dtschump]

One other idea I would have is to allow increasing the size of the font cache?

Doing do would require a cache reallocation from time to time, breaking also all existing references to previous cache buffer.

For now, I think the best approach is to protect the ‘draw_text(...,font_height)’ function with a mutex rather than forcing a font copy each time it is used. And suggest in the documentation to use the draw_text(...,CImgList<T> &font) versions, which are not protected by mutex, for those who really want to be able to write text in multi-threaded mode (but leaving it up to them to have their own copy of the fonts they use).
This seems to me to be the best compromise.

[dtschump]

Latest commit 863bdba should fix this issue.