Bug 6717 - Crash on NULL surface
Summary: Crash on NULL surface
Status: RESOLVED INVALID
Alias: None
Product: cairo
Classification: Unclassified
Component: xlib backend (show other bugs)
Version: 1.1.1
Hardware: x86 (IA32) Linux (All)
: high normal
Assignee: Carl Worth
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-04-24 17:49 UTC by Hans Petter Jansson
Modified: 2006-04-24 18:18 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Patch that fixed the crashes for me. (1.46 KB, patch)
2006-04-24 17:49 UTC, Hans Petter Jansson
Details | Splinter Review

Description Hans Petter Jansson 2006-04-24 17:49:31 UTC
Ran across this bug in cairo HEAD. Nautilus (the GNOME file manager) was showing
some filenames in an asian script, and it crashed deep in Cairo code, first in
one place, then in another. I patched both places to check for a NULL surface,
and that fixed the problem. Things seemed to render fine, without any crashes.

I'm attaching the (tiny) patch. It may not be correct, but at least it gives a
good indication of what needs to be fixed.

Note that it's very easy to prove that *surface can be NULL in
_render_glyph_bitmap(). The call to _get_bitmap_surface() may set it to NULL.
Comment 1 Hans Petter Jansson 2006-04-24 17:49:59 UTC
Created attachment 5445 [details] [review]
Patch that fixed the crashes for me.
Comment 2 Carl Worth 2006-04-25 03:48:32 UTC
(In reply to comment #0)
> Note that it's very easy to prove that *surface can be NULL in
> _render_glyph_bitmap(). The call to _get_bitmap_surface() may set it to NULL.

NULL surface pointers are definitely a sign of some internal cairo bug. The code
should generally use non-NULL pointers to special "nil" surface objects instead.

And as far as I can see, that's mostly what should be happening here. For
example, in _get_bitmap_surface, (as of the latest checkout from git), the only
explicit assignment to *surface is:

    *surface = (cairo_image_surface_t *)
        cairo_image_surface_create_for_data (data,
                                             format,
                                             width, height, stride);
    if ((*surface)->base.status) {
        free (data);
        return CAIRO_STATUS_NO_MEMORY;
    }

Here cairo_image_surface_create_for_data is specified as never returning NULL.
And if it did, then the code immediately following would trigger a segfault.

Meanwhile, there are three return statements in _get_bitmap_surface that
will return without assigning *surface any value at all. However, in each case
an error value is being returned, (CAIRO_STATUS_NO_MEMORY), and the caller
should then not be looking at the surface pointer at all.

I checked the two immediate callers to _get_bitmap_surface and they are
examining the return value and bailing out on error. I did not follow the call
tree up higher to see if some caller is neglecting to check a return value.

Since you've presumably got a way to replicate this bug, it might be easier for
you to find a missing check along the call tree there.

Or maybe the NULL value is coming from somewhere other than _get_bitmap_surface.
I only looked there because it's where you pointed to.

If you can't figure out where the invalid NULL is coming from, then a minimal
test case that replicates the bug would be quite helpful if I were to look any
closer.

Thanks,

-Carl
Comment 3 Hans Petter Jansson 2006-04-25 06:38:24 UTC
Hi Carl,

It sounds like we have different versions of the source. In mine, which is
up-to-date with anoncvs.freedesktop.org, cairo-ft-font.c:_get_bitmap_surface()
has this close to the top:

    if (width * height == 0) {
	if (own_buffer && bitmap->buffer)
	    free (bitmap->buffer);
	
	*surface = NULL;
    } else {

Later on, the function exits with a SUCCESS value. If this means I don't have a
current checkout, feel free to close the bug. Otherwise, I can put together a
test case.
Comment 4 Carl Worth 2006-04-25 07:11:54 UTC
OK. You do have stale code. The stuff in CVS is not up to date (and I need to
fix it so that it advertises itself LOUDLY that it is stale).

See the following page for information on pulling the latest source down with git:

http://cairographics.org/download

And do let me know if the bug is gone in the latest.

Sorry about the version confusion (which is definitely my fault).

-Carl
Comment 5 Hans Petter Jansson 2006-04-25 08:45:13 UTC
I got the current tree from git, and it works just fine based on my limited
testing. Closing the bug.

Making the CVS version advertise itself as stale is a good idea. GNOME
developers  who use jhbuild to keep up to date (which is pretty much all of us)
will get the CVS version currently, which means Cairo HEAD is not benefiting
from our testing. I guess we need to fix that (in jhbuild) too.
Comment 6 Carl Worth 2006-04-25 08:56:34 UTC
Yes, the jhbuild issue is a known issue since day 1 of the switch to git.

I really do want cairo to be able to benefit from GNOME testing, so my original
intent had been to provide a git->cvs bridge to allow jhbuild to continue to
work as is.

I recently decided this wasn't going to work out:

http://lists.freedesktop.org/archives/cairo/2006-April/006705.html

So my current plan is to get a tar file snapshot out today, encourage the
jhbuild maintainers to point at that for cairo, and then fix the CVS checkout to
advertise itself as stale.

Meanwhile, I've got several ideas for teaching jhbuild to talk to git if someone
is interested in doing that work---just email me if you know of anyone.

Thanks,

-Carl
Comment 7 Behdad Esfahbod 2006-04-25 11:18:57 UTC
Here is the bug to add git support to jhbuild:

  http://bugzilla.gnome.org/show_bug.cgi?id=333425

It has a draft patch.


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.