[cairo-bugs] [Bug 91967] Assertion "(_cairo_atomic_int_get (&(&surface->ref_count)->ref_count) > 0)"

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=91967

--- Comment #7 from Alberts Muktupāvels <alberts.muktupavels at gmail.com> ---
(In reply to Bryce Harrington from comment #6)
> A couple things...
> 
> First, if we set image to NULL and it hits BAIL, then doesn't it just crash
> unreferencing it here?
> 
>     if (unlikely (status)) {
>         cairo_surface_destroy (&image->base);
>         return _cairo_surface_create_in_error (status);
>     }

No. Without NULL it crash here because it tries to destroy image->base for
second time...

> If image can be NULL, then there are a few places in the function where
> &image->base is used that would need NULL ptr checks else they might
> segfault.

If I know how to read code then this is not going to happen. When image->base
is used again it is first re-created. Line 896 and 969. I fail to see how this
could happen.

> As well, I would think the image object would need to be deliberately
> destroyed, to avoid memory leak.  I.e.:
> 
> ...
>         cairo_surface_destroy (&image->base);
>         cairo_surface_destroy (image);
>         image = NULL;
>     }
> ...

Since I have no idea how cairo works I decided to test this. This just causes
another assert with ref_count > 0 failed. So this is wrong. Setting it to NULL
is enough.

> However, backing up a bit... if XShmGetImage() has failed, such that we're
> destroying &image->base, is any of the rest of the function still valid?  I

Yes function is still valid.

> notice from your trace that it's just hitting another error and bouncing
> down to BAIL, where the crash occurs.  It looks like most of the BAIL logic
> doesn't apply here since the objects its freeing hadn't been created. Which

Wrong. It does not try to free object that hadn't been created. It tries to
free object that was created and then freed.

> makes me wonder if when this error situation is hit, if it'd be better to
> just exit the function?  Then we can avoid worrying about NULL image
> pointers and what's happening in BAIL.
> 
> ...
>         cairo_surface_destroy (&image->base);
>         cairo_surface_destroy (image);
>         return _cairo_surface_create_in_error (CAIRO_STATUS_INVALID_VISUAL);
>     }
> ...
> 
> Maybe the error code there should be CAIRO_STATUS_NO_MEMORY instead.

Again, here is what happens:
1) 'if (try_shm && pixman_format)' block creates image, but XShmGetImage fails
so we destroy it. Now image points to invalid / freed object.
2) It ties to get/create ximage, but it fails too so we end up in 'if (ximage
== NULL)' block (line 880), it sets status to NO_MEMORY and goto BAIL. (If
ximage != NULL then image will be re-created, bet that is not my case.)
3) Now we at line 1008 (BAIL). We have set error so we enter 'if (unlikely
(status))'. And what we try to do here? We try to destroy already destroyed
image->base. And thats is my problem.

Since your suggestion to use cairo_surface_destroy (image); ends up with other
ref_count assert (basically double free) I assume that there is no memory leaks
and it is properly freed. So ONLY problem is that image points to destroyed
object.

Reading code I see one more 'goto BAIL' (line 967) that could lead to same
problem/error/crash. In all other cases image is recreated.

Do you need more info and/or want that I test any other possible solutions? I
think now it should be clear what happens and that setting image to NULL is
safe (at least I fail to see how that could cause problem).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cairographics.org/archives/cairo-bugs/attachments/20151016/dcfdeac4/attachment-0001.html>