Bug 58061 - Cairo should not return NULL surfaces or devices to clients. (Was: Null pointer dereference in cairo_image_get_surface_data())
Summary: Cairo should not return NULL surfaces or devices to clients. (Was: Null poin...
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: win32 backend (show other bugs)
Version: 1.12.8
Hardware: All Windows (All)
: medium normal
Assignee: cairo-bugs mailing list
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: cairo-1.14
  Show dependency tree / graph
 
Reported: 2012-12-09 20:56 UTC by mov_ebpesp
Modified: 2014-09-23 20:05 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Attachments
Detailed information of the crash. (75.88 KB, text/plain)
2012-12-09 20:56 UTC, mov_ebpesp 		Details
List of places that might return NULL (2.94 KB, patch)
2013-09-25 18:32 UTC, Uli Schlachter 		Details | Splinter Review
View All

Description mov_ebpesp 2012-12-09 20:56:29 UTC 
Created attachment 71241 [details]
Detailed information of the crash.

Bug found while running gnucash 2.4.11 on windows (cross bug from gnucash https://bugzilla.gnome.org/show_bug.cgi?id=689942).

Facts
-----

Every now and then (once per 1-3 h) gnucash seems to crash. This can happen
after a save, or updating a transaction or even leaving the computer alone for
a while - when I come back it's crashed.

The crash is a null pointer exception inside
libcairo.cairo_image_get_surface_data where a comparison check is done against
0.

68DE04E1  |.  8138 20AEE968 CMP DWORD PTR DS:[EAX],libcairo.68E9AE20

libcairo.68E9AE20 is just 00 00 00 00

This bug can be fixed by first checking EAX for 0 and then checking the pointer
pointed to by EAX if it's 0.

See attached "crash.txt" for more details.

Proposed fix (credits to John Ralls from gnucash team)
------------------------------------------------------

The actual patch is:
diff --git a/src/cairo-image-surface-inline.h
b/src/cairo-image-surface-inline.h
index 743d5fd..63e0c50 100644
--- a/src/cairo-image-surface-inline.h
+++ b/src/cairo-image-surface-inline.h
@@ -74,7 +74,7 @@ static inline cairo_bool_t
 _cairo_surface_is_image (const cairo_surface_t *surface)
 {
     /* _cairo_surface_nil sets a NULL backend so be safe */
-    return surface->backend && surface->backend->type ==
CAIRO_SURFACE_TYPE_IMA
+    return surface && surface->backend && surface->backend->type ==
CAIRO_SURFA
 }
Comment 1 Chris Wilson 2012-12-09 21:05:14 UTC 
Ages ago the design was made not to work around extremely broken applications - Cairo never returns a NULL pointer to the application, so it must be injecting garbage into the library and deserves to be shot.
Comment 2 John Ralls 2012-12-09 22:20:39 UTC 
(In reply to comment #1)
> Ages ago the design was made not to work around extremely broken
> applications - Cairo never returns a NULL pointer to the application, so it
> must be injecting garbage into the library and deserves to be shot.

Really? From src/win32/cairo-win32-surface.c in git master:

cairo_surface_t *
cairo_win32_surface_get_image (cairo_surface_t *surface)
{
    if (surface->backend->type != CAIRO_SURFACE_TYPE_WIN32)
        return NULL;

    GdiFlush();
    return to_win32_display_surface(surface)->image;
}
Comment 3 Uli Schlachter 2013-09-25 18:32:01 UTC 
Created attachment 86585 [details] [review]
List of places that might return NULL

I went through the public headers and looked for places where cairo might return NULL for a surface or a device. I don't claim that this list is complete, but it certainly seems like a good start to me.
Comment 4 Bryce Harrington 2014-09-16 18:31:52 UTC 
So, it sounds like the "fix" in this case is for gnucash to check the surface is not NULL before using it in cairo calls.

Uli points out a few places where cairo does indeed return NULLs to the calling user:

1. cairo_win32_surface_get_image, cairo_quartz_image_surface_get_image and cairo_qt_surface_get_image do this to indicate errors if the surface type is incorrect.  These are of type cairo_surface_t*.  We should be returning an error surface via _cairo_surface_create_in_error() instead.

2. Similarly, in cairo_cogl_device_create, cairo_drm_device_get, and (probably) cairo_drm_device_default, instead of returning NULL for the error cases the routines should generate an error device via _cairo_device_create_in_error() instead.

Somewhere in the documentation we may want to mention that passing in NULL pointers for surface arguments is not kosher.  I'm not sure we'd want to litter the API docs with "surface argument required; must be non-NULL".
Comment 5 Uli Schlachter 2014-09-23 20:05:48 UTC 
commit 150c1e7044c57443d458e12bfc427d3a019cb60b
Author: Bryce Harrington <bryce@osg.samsung.com>
Date:   Mon Sep 22 15:41:24 2014 -0700

    Don't return NULL to clients when getting image
    
    Return an error surface instead.
    
    Fixes:  https://bugs.freedesktop.org/show_bug.cgi?id=58061

commit a02e29a12d82f177bdf99ed8cfd0c3b3b78c44da
Author: Bryce Harrington <bryce@osg.samsung.com>
Date:   Mon Sep 22 15:48:49 2014 -0700

    Don't return NULL to clients when getting device
    
    Return an error device instead
    
    Fixes:  https://bugs.freedesktop.org/show_bug.cgi?id=58061

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.