Skip to content

Fix memory leak in Context::pop_group() due to double reference count#1

Open
hana-kami wants to merge 1 commit intoCMakePorts:cmakefrom
hana-kami:cmake
Open

Fix memory leak in Context::pop_group() due to double reference count#1
hana-kami wants to merge 1 commit intoCMakePorts:cmakefrom
hana-kami:cmake

Conversation

@hana-kami
Copy link
Copy Markdown

Bug Description

Context::pop_group() causes a memory leak because it passes
has_reference=false to get_pattern_wrapper(), which unconditionally
calls cairo_pattern_reference() to take an extra reference. However,
cairo_pop_group() already returns a pattern with an initial refcount of
1 — the caller owns that reference. This results in the refcount being
inflated by 1 and never reaching zero.

Reference Count Trace (before fix)

Step Operation refcount
1 cairo_pop_group() returns pattern (caller owns it) 1
2 get_pattern_wrapper(pattern, false) → calls cairo_pattern_reference() 2
3 cairo_set_source() inside set_source() adds a reference 3
4 Temporary RefPtr<Pattern> is destroyed → cairo_pattern_destroy() 2
5 Context replaces source (e.g. on restore()) → cairo_pattern_destroy() 1
6 No pointer to the pattern remains → definite leak 1 (leaked)

This was confirmed with Valgrind:

21,960 bytes in 9 blocks are definitely lost
  at cairo_pattern_create_for_surface (cairo-pattern.c:771)
  by _cairo_default_context_pop_group (cairo-default-context.c:238)
  by cairo_pop_group (cairo.c:768)
  by Cairo::Context::pop_group() (context.cc:885)

Root Cause

get_pattern_wrapper() was designed for get_source(), where
cairo_get_source() returns a borrowed reference (the context still
owns it), so the wrapper must call cairo_pattern_reference() to obtain
its own reference (has_reference=false is correct there).

But cairo_pop_group() returns a new reference (the caller is
responsible for calling cairo_pattern_destroy()), so the wrapper must
NOT call cairo_pattern_reference() again.

Fix

Add a has_reference parameter (defaulting to false) to
get_pattern_wrapper() so callers can specify whether they already own
the reference. Update pop_group() to pass true.

  • get_source() continues to use the default false (borrowed reference → correct).
  • pop_group() now passes true (owned reference → no extra ref).

Testing

Verified with Valgrind that the "definitely lost" leak no longer appears
after applying this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant