Bug 4198 - ref_count fields can go negative
Summary: ref_count fields can go negative
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: 0.9.3
Hardware: x86 (IA32) Linux (All)
: high normal
Assignee: Carl Worth
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-22 17:19 UTC by Carl Worth
Modified: 2005-08-22 21:04 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Treat a 0 reference count same as -1 in reference and destroy functions (4.40 KB, patch)
2005-08-23 00:12 UTC, Carl Worth
Details | Splinter Review
Crash by assert if ref_count == 0 in reference or destroy. (4.47 KB, patch)
2005-08-23 13:21 UTC, Carl Worth
Details | Splinter Review
Fixed version (missing semicolon) (4.44 KB, patch)
2005-08-23 13:44 UTC, Carl Worth
Details | Splinter Review

Description Carl Worth 2005-08-22 17:19:08 UTC
The various ref_count fields will go negative if destroy is called with
ref_count == 0. We noticed this in cairo_surface.c but it looks like a
more general bug pattern.

Since we first noticed this bug, we added special handling for
ref_count == -1 for nil objects. So we just need a simple extension
of that handling to cover this case as we..
Comment 1 Carl Worth 2005-08-23 00:12:07 UTC
Created attachment 3002 [details] [review]
Treat a 0 reference count same as -1 in reference and destroy functions

This patch changes the common pattern of:

    if (object->ref_count == (unsigned int)-1)
	return;

that appears in all object_reference and corresponding object_destroy functions
to:

    if (object->ref_count <= 0)
	return;

This should catch and stop the 0 ref_count from changing. It also makes all
negative ref_count values equivalently no-ops, which I don't think should
hurt anything.
Comment 2 Carl Worth 2005-08-23 13:21:43 UTC
Created attachment 3009 [details] [review]
Crash by assert if ref_count == 0 in reference or destroy.

Owen pointed out that detecting this error and then silently doing nothing
is not helpful.

We can't use standard cairo error reporting since we're detecting garbage
objects
in these cases. So this patch just uses assert to crash if the user ever does
this.

It would probably be even better to print a helpful message to the user
in this case.
Comment 3 Carl Worth 2005-08-23 13:44:00 UTC
Created attachment 3010 [details] [review]
Fixed version (missing semicolon)

Same as previous patch, but this one actually compiles, (fixed missing
semicolon).
Comment 4 Carl Worth 2005-08-23 14:04:45 UTC
2005-08-23  Carl Worth  <cworth@cworth.org>

        Reviewed by: otaylor
        Fixes bug #4198

        * src/cairo-font.c: (cairo_font_face_reference),
        (cairo_font_face_destroy), (cairo_scaled_font_reference),
        (cairo_scaled_font_destroy):
        * src/cairo-pattern.c: (cairo_pattern_reference),
        (cairo_pattern_destroy):
        * src/cairo-surface.c: (cairo_surface_reference),
        (cairo_surface_destroy):
        * src/cairo.c: (cairo_reference), (cairo_destroy):
        Detect (by assert and crash) if users attempt to twice destroy or
        re-reference a destroyed object. The condition for detecting this
        case is a ref_count of 0.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.