[cairo-commit] 6 commits - src/cairo-ft-font.c src/cairo-gstate.c src/cairo-scaled-font.c src/cairo-script-surface.c src/cairo-surface.c test/leaks.c test/meson.build test/reference
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Mon Sep 18 17:40:54 UTC 2023
src/cairo-ft-font.c | 1
src/cairo-gstate.c | 1
src/cairo-scaled-font.c | 8 +++
src/cairo-script-surface.c | 1
src/cairo-surface.c | 4 +
test/leaks.c | 62 +++++++++++++++++++++++++++
test/meson.build | 1
test/reference/leaks-set-scaled-font.ref.png |binary
8 files changed, 78 insertions(+)
New commits:
commit c45e373fb457c0a1829e5c945461aa496d937728
Merge: 7380d3dd7 06864022c
Author: Emmanuele Bassi <ebassi at gmail.com>
Date: Mon Sep 18 17:40:52 2023 +0000
Merge branch 'font-option-leaks' into 'master'
Fix font options leak in gstate
Closes #795
See merge request cairo/cairo!512
commit 06864022c88d631d0c54b0b6756b157dbe30275c
Author: Uli Schlachter <psychon at znc.in>
Date: Sun Sep 17 10:00:51 2023 +0200
Fix font options leak in cairo script surface
I added options->variations = strdup("slnt=0,wght=400,wdth=100"); to the
end of _cairo_font_options_init_default(). This makes all font option
objects own some memory that needs to be freed. Then I ran some random
test under valgrind and found memory leaks.
This commit makes the script surface finish the font options that it
contains. This fixes the following valgrind report:
25 bytes in 1 blocks are definitely lost in loss record 8 of 21
at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4ECBC99: strdup (strdup.c:42)
by 0x4886B7F: _cairo_font_options_init_default (cairo-font-options.c:86)
by 0x49768F4: _cairo_script_implicit_context_init (cairo-script-surface.c:3676)
by 0x4976B22: _cairo_script_surface_create_internal (cairo-script-surface.c:3733)
by 0x4976EA1: cairo_script_surface_create (cairo-script-surface.c:3962)
by 0x1B0A97: _cairo_boilerplate_script_create_surface (cairo-boilerplate-script.c:63)
by 0x129B7F: cairo_test_for_target (cairo-test.c:824)
by 0x12B37F: _cairo_test_context_run_for_target (cairo-test.c:1545)
by 0x12C385: _cairo_test_runner_draw (cairo-test-runner.c:258)
by 0x12DEB5: main (cairo-test-runner.c:962)
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
index 1175bd8f3..ca4db5a69 100644
--- a/src/cairo-script-surface.c
+++ b/src/cairo-script-surface.c
@@ -2204,6 +2204,7 @@ _cairo_script_surface_finish (void *abstract_surface)
_cairo_pattern_fini (&surface->cr.current_source.base);
_cairo_path_fixed_fini (&surface->cr.current_path);
+ _cairo_font_options_fini (&surface->cr.current_font_options);
_cairo_surface_clipper_reset (&surface->clipper);
status = cairo_device_acquire (&ctx->base);
commit 29087868cd28fc95ced901f2f6b35a9ca27b615a
Author: Uli Schlachter <psychon at znc.in>
Date: Sun Sep 17 09:51:51 2023 +0200
Fix font options leak in _cairo_surface_copy_similar_properties()
I added options->variations = strdup("slnt=0,wght=400,wdth=100"); to the
end of _cairo_font_options_init_default(). This makes all font option
objects own some memory that needs to be freed. Then I ran some random
test under valgrind and found memory leaks.
_cairo_surface_copy_similar_properties() gets the font options of a
surface via cairo_surface_get_font_options(). This creates a copy of the
font variations that I added above. _cairo_surface_set_font_options()
then copies this again (it calls _cairo_font_options_init_copy). Thus,
the original copy is still owned by
_cairo_surface_copy_similar_properties() and needs to be freed.
This commit fixes four leaks in "valgrind --leak-check=full
./cairo-test-suite -f leaks-set-scaled-font". A random example is:
25 bytes in 1 blocks are definitely lost in loss record 4 of 25
at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4ECBC99: strdup (strdup.c:42)
by 0x4886C0C: _cairo_font_options_init_copy (cairo-font-options.c:99)
by 0x48F1DDE: cairo_surface_get_font_options (cairo-surface.c:1620)
by 0x48F0691: _cairo_surface_copy_similar_properties (cairo-surface.c:454)
by 0x48F087C: cairo_surface_create_similar (cairo-surface.c:528)
by 0x1B168A: _cairo_boilerplate_pdf_create_surface (cairo-boilerplate-pdf.c:92)
by 0x129B7F: cairo_test_for_target (cairo-test.c:824)
by 0x12B37F: _cairo_test_context_run_for_target (cairo-test.c:1545)
by 0x12C385: _cairo_test_runner_draw (cairo-test-runner.c:258)
by 0x12DEB5: main (cairo-test-runner.c:962)
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 155f43882..c208f99f2 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -453,6 +453,7 @@ _cairo_surface_copy_similar_properties (cairo_surface_t *surface,
cairo_surface_get_font_options (other, &options);
_cairo_surface_set_font_options (surface, &options);
+ _cairo_font_options_fini (&options);
}
cairo_surface_set_fallback_resolution (surface,
commit 4c1987b0f090586f8c7fa5b4ceaf4f9b0bf66999
Author: Uli Schlachter <psychon at znc.in>
Date: Sun Sep 17 09:45:04 2023 +0200
Fix font options leak in cairo-surface.c
When calling cairo_surface_get_font_options(), a font options instance
is allocated for the surface. Normally, this just initialised some
otherwise uninitialised fields in cairo_surface_t. Since commit
67eeed44, cairo_font_options_t can contain an extra allocation for a
custom palette. Since commit edf9497c3a, cairo_font_options_t can
contain an extra allocation for a string. Before these commit, font
options could just be dropped, but now they need to be freed.
This commit makes cairo_surface_destroy() finish the contained font
options if they were initialised.
I didn't manage to produce a self-contained test case for this leak. I
found it by just looking at the code. However, I found a way to force a
leak: By adding options->variations=strdtup("slnt=0,wght=400,wdth=100");
to the end of _cairo_font_options_init_default(), all font option
instances now cause a leak unless they are finished. With this extra
change, this commit fixes a memory leak that is simply caused by calling
cairo_surface_get_font_options().
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 9110edb1a..155f43882 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -983,6 +983,9 @@ cairo_surface_destroy (cairo_surface_t *surface)
if (surface->owns_device)
cairo_device_destroy (surface->device);
+ if (surface->has_font_options)
+ _cairo_font_options_fini (&surface->font_options);
+
assert (surface->snapshot_of == NULL);
assert (! _cairo_surface_has_snapshots (surface));
/* paranoid check that nobody took a reference whilst finishing */
commit 7bf743a92fb10a18473accd5ae3c9005debed1c5
Author: Uli Schlachter <psychon at znc.in>
Date: Sun Sep 17 09:32:10 2023 +0200
Fix font options leak in scaled font
A scaled font contains font options. Since commit 67eeed44, this can
contain an extra allocation for a custom palette. Since commit
edf9497c3a, this contains an extra allocation for a string. Before these
commit, font options could just be dropped, but now they need to be
freed.
This commit makes the relevant code for creating and finishing scaled
fonts also clean up the font options.
The test added in the previous commit also hits this bug (I only found
these leaks accidentially!). Running "valgrind --leak-check=full
./cairo-test-suite -f leaks-set-scaled-font" no longer reports the following
after this change:
40 bytes in 1 blocks are definitely lost in loss record 1 of 11
at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4886C62: _cairo_font_options_init_copy (cairo-font-options.c:105)
by 0x48DAFFB: _cairo_scaled_font_init_key (cairo-scaled-font.c:675)
by 0x48DC077: cairo_scaled_font_create (cairo-scaled-font.c:1096)
by 0x15BF08: leaks_set_scaled_font (leaks.c:43)
by 0x129EF0: cairo_test_for_target (cairo-test.c:938)
by 0x12B37F: _cairo_test_context_run_for_target (cairo-test.c:1545)
by 0x12C385: _cairo_test_runner_draw (cairo-test-runner.c:258)
by 0x12DEB5: main (cairo-test-runner.c:962)
40 bytes in 1 blocks are definitely lost in loss record 2 of 11
at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4886C62: _cairo_font_options_init_copy (cairo-font-options.c:105)
by 0x49337BB: _cairo_ft_font_face_scaled_font_create (cairo-ft-font.c:2073)
by 0x48DC340: cairo_scaled_font_create (cairo-scaled-font.c:1176)
by 0x15BF08: leaks_set_scaled_font (leaks.c:43)
by 0x129EF0: cairo_test_for_target (cairo-test.c:938)
by 0x12B37F: _cairo_test_context_run_for_target (cairo-test.c:1545)
by 0x12C385: _cairo_test_runner_draw (cairo-test-runner.c:258)
by 0x12DEB5: main (cairo-test-runner.c:962)
40 bytes in 1 blocks are definitely lost in loss record 3 of 11
at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4886C62: _cairo_font_options_init_copy (cairo-font-options.c:105)
by 0x48DB280: _cairo_scaled_font_init (cairo-scaled-font.c:742)
by 0x4933804: _cairo_ft_font_face_scaled_font_create (cairo-ft-font.c:2076)
by 0x48DC340: cairo_scaled_font_create (cairo-scaled-font.c:1176)
by 0x15BF08: leaks_set_scaled_font (leaks.c:43)
by 0x129EF0: cairo_test_for_target (cairo-test.c:938)
by 0x12B37F: _cairo_test_context_run_for_target (cairo-test.c:1545)
by 0x12C385: _cairo_test_runner_draw (cairo-test-runner.c:258)
by 0x12DEB5: main (cairo-test-runner.c:962)
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index 21d6c6193..bf0872e93 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -2173,6 +2173,7 @@ _cairo_ft_scaled_font_fini (void *abstract_font)
if (scaled_font == NULL)
return;
+ _cairo_font_options_fini (&scaled_font->ft_options.base);
_cairo_unscaled_font_destroy (&scaled_font->unscaled->base);
}
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index 58919e215..3a0988899 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -678,6 +678,12 @@ _cairo_scaled_font_init_key (cairo_scaled_font_t *scaled_font,
_cairo_scaled_font_compute_hash (scaled_font);
}
+static void
+_cairo_scaled_font_fini_key (cairo_scaled_font_t *scaled_font)
+{
+ _cairo_font_options_fini (&scaled_font->options);
+}
+
static cairo_bool_t
_cairo_scaled_font_keys_equal (const void *abstract_key_a,
const void *abstract_key_b)
@@ -914,6 +920,7 @@ _cairo_scaled_font_fini_internal (cairo_scaled_font_t *scaled_font)
_cairo_scaled_font_reset_cache (scaled_font);
_cairo_hash_table_destroy (scaled_font->glyphs);
+ _cairo_font_options_fini (&scaled_font->options);
cairo_font_face_destroy (scaled_font->font_face);
cairo_font_face_destroy (scaled_font->original_font_face);
@@ -1105,6 +1112,7 @@ cairo_scaled_font_create (cairo_font_face_t *font_face,
* just wait until it's done, then retry */
_cairo_scaled_font_placeholder_wait_for_creation_to_finish (scaled_font);
}
+ _cairo_scaled_font_fini_key (&key);
if (scaled_font != NULL) {
/* If the original reference count is 0, then this font must have
diff --git a/test/leaks.c b/test/leaks.c
index 612d4a1e6..ad7c85817 100644
--- a/test/leaks.c
+++ b/test/leaks.c
@@ -27,7 +27,8 @@
#include "cairo-test.h"
-// Once upon a time, _cairo_gstate_fini leaked font options
+// Once upon a time, _cairo_gstate_fini(), _cairo_scaled_font_fini_internal(),
+// and _cairo_ft_scaled_font_fini() leaked font options.
static cairo_test_status_t
leaks_set_scaled_font (cairo_t *cr, int width, int height)
{
commit 9529d02f6aecb234c1e4aaffd972eb439a74fb9a
Author: Uli Schlachter <psychon at znc.in>
Date: Sun Sep 17 09:22:29 2023 +0200
Fix font options leak in gstate
cairo_gstate_t contains a cairo_font_options_t. Since commit 67eeed44,
this can contain an extra allocation for a custom palette. Since commit
edf9497c3a, this contains an extra allocation for a string. Before these
commit, font options could just be dropped, but now they need to be
freed.
This commit makes _cairo_gstate_fini() finish the font options to free
the memory allocation.
The new test was run via "valgrind --leak-check=full ./cairo-test-suite
-f leaks-set-scaled-font". The following reported leak goes away thanks
to this commit:
1,040 bytes in 26 blocks are definitely lost in loss record 6 of 12
at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4886C62: _cairo_font_options_init_copy (cairo-font-options.c:105)
by 0x488C029: _cairo_gstate_set_font_options (cairo-gstate.c:1757)
by 0x48841D7: _cairo_default_context_set_scaled_font (cairo-default-context.c:1310)
by 0x490809A: cairo_set_scaled_font (cairo.c:3318)
by 0x15BF1F: leaks_set_scaled_font (leaks.c:45)
by 0x129EF0: cairo_test_for_target (cairo-test.c:938)
by 0x12B37F: _cairo_test_context_run_for_target (cairo-test.c:1545)
by 0x12C385: _cairo_test_runner_draw (cairo-test-runner.c:258)
by 0x12DEB5: main (cairo-test-runner.c:962)
Fixes: https://gitlab.freedesktop.org/cairo/cairo/-/issues/795
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/cairo-gstate.c b/src/cairo-gstate.c
index 8a253468d..e9019e8e6 100644
--- a/src/cairo-gstate.c
+++ b/src/cairo-gstate.c
@@ -193,6 +193,7 @@ void
_cairo_gstate_fini (cairo_gstate_t *gstate)
{
_cairo_stroke_style_fini (&gstate->stroke_style);
+ _cairo_font_options_fini (&gstate->font_options);
cairo_font_face_destroy (gstate->font_face);
gstate->font_face = NULL;
diff --git a/test/leaks.c b/test/leaks.c
new file mode 100644
index 000000000..612d4a1e6
--- /dev/null
+++ b/test/leaks.c
@@ -0,0 +1,61 @@
+/*
+ * Copyright © 2023 Uli Schlachter
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy,
+ * modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Author: Uli Schlachter <psychon at znc.in>
+ */
+
+#include "cairo-test.h"
+
+
+// Once upon a time, _cairo_gstate_fini leaked font options
+static cairo_test_status_t
+leaks_set_scaled_font (cairo_t *cr, int width, int height)
+{
+ cairo_font_options_t *opt;
+ cairo_matrix_t matrix;
+ cairo_scaled_font_t *font;
+
+ cairo_matrix_init_identity (&matrix);
+
+ opt = cairo_font_options_create ();
+ cairo_font_options_set_custom_palette_color (opt, 0, 1, 1, 1, 1);
+
+ font = cairo_scaled_font_create (cairo_get_font_face (cr), &matrix, &matrix, opt);
+
+ cairo_set_scaled_font (cr, font);
+
+ cairo_font_options_destroy (opt);
+ cairo_scaled_font_destroy (font);
+
+ // Fill the output so that the same ref image works for everying
+ cairo_paint (cr);
+
+ return CAIRO_TEST_SUCCESS;
+}
+
+CAIRO_TEST (leaks_set_scaled_font,
+ "Regression test for font options memory leak in cairo_set_scaled_font",
+ "leak", /* keywords */
+ NULL, /* requirements */
+ 1, 1,
+ NULL, leaks_set_scaled_font)
diff --git a/test/meson.build b/test/meson.build
index 1d76d5daa..70f517f3c 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -190,6 +190,7 @@ test_sources = [
'large-source.c',
'large-source-roi.c',
'large-twin-antialias-mixed.c',
+ 'leaks.c',
'leaky-dash.c',
'leaky-dashed-rectangle.c',
'leaky-dashed-stroke.c',
diff --git a/test/reference/leaks-set-scaled-font.ref.png b/test/reference/leaks-set-scaled-font.ref.png
new file mode 100644
index 000000000..7d5589c1d
Binary files /dev/null and b/test/reference/leaks-set-scaled-font.ref.png differ
More information about the cairo-commit
mailing list