Skip to content

Fix usage of FT_Face after FT_Done_Face and unsynchronised access to FT_Library and num_open_faces

Michal requested to merge miso/cairo:fix_use_after_free_race into master

Probably fixes #436. Access to unscaled in cairo_ft_unscaled_font_map is not synchronised between _cairo_ft_unscaled_font_lock_face and _font_map_release_face_lock_held called from within it but for another unscaled used by another random thread. First thread may fetch unscaled with lock count zero under font map mutex then second thread increments this lock count and immediately returns face under unscaled mutex and then first thread releases this face by FT_Done_Face and zeroes face in unscaled but face was already returned and will be used after it was freed. Also such unscaled with NULL face is used which may trigger the assertion mentioned in #436.

Another problem may be concurrent call to FT_New_Face with shared font_map->ft_library without lock as is mentioned in https://github.com/behdad/ftthread. And concurrent non-atomic incrementation of font_map->num_open_faces which causes assertion in _cairo_ft_unscaled_font_map_destroy.

This patch extends the font map lock to cover all these accesses.

Here is a test case. It always crashes within a second before patch but does not crash after. It needs the package fonts-dejavu. It also does not crash with current cairo when the number of fonts is reduced to 10 or MAX_OPEN_FACES is increased to 16.

#include <vector>
#include <thread>
#include <pango/pangocairo.h>
#include <cairo.h>

int main()
{
	auto fonts = {
		"DejaVu Sans", "DejaVu Sans Bold", "DejaVu Sans Oblique", "DejaVu Sans Bold Oblique",
		"DejaVu Sans Mono", "DejaVu Sans Mono Bold", "DejaVu Sans Mono Oblique", "DejaVu Sans Mono Bold Oblique",
		"DejaVu Sans Condensed", "DejaVu Sans Condensed Bold", "DejaVu Sans Condensed Oblique", "DejaVu Sans Condensed Bold Oblique",
		"DejaVu Serif", "DejaVu Serif Bold", "DejaVu Serif Oblique", "DejaVu Serif Bold Oblique"
	};
	while(true)
	{
		std::vector<std::thread> threads;
		for(auto fnt : fonts)
			threads.emplace_back([=]()
			{
				auto surface = cairo_image_surface_create(CAIRO_FORMAT_RGB24, 100, 100);
				auto ctx = cairo_create(surface);
				auto layout = pango_cairo_create_layout(ctx);
				auto desc = pango_font_description_from_string(fnt);
				pango_layout_set_font_description(layout, desc);
				pango_layout_set_text(layout, "Text", -1);
				pango_cairo_show_layout(ctx, layout);
				cairo_show_page(ctx);
				pango_font_description_free(desc);
				g_object_unref(layout);
				cairo_destroy(ctx);
				cairo_surface_destroy(surface);
			});
		for(auto &th : threads)th.join();
		cairo_debug_reset_static_data();
	}
	return 0;
}

An alternative could be to use unscaled mutex before calling _font_map_release_face_lock_held in while loop but thread sanitizer warns against possible deadlock. First thread may lock mutex of some unscaled then second thread locks font map mutex and fetches same unscaled before is lock count incremented and tries to lock its mutex before _font_map_release_face_lock_held but it must wait as this mutex is already locked by first thread and then first thread tries to lock font map mutex which is locked by second thread thus resulting in deadlock. Of course this should never happen because the first thread never tries to lock the font map mutex if unscaled has face and the second thread never fetches unscaled (entry) from font map which does not have face. But to me such precondition looks really fragile and unnecessarily complex.

Edited by Michal

Merge request reports