Bug 89521

Summary: segmentation fault during poppler_page_render (crashes inside _fill_xrgb32_lerp_opaque_spans)
Product: cairo Reporter: draymond
Component: generalAssignee: Chris Wilson <chris>
Status: RESOLVED MOVED QA Contact: cairo-bugs mailing list <cairo-bugs>
Severity: critical    
Priority: high CC: dmoppert
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: (PDF file used in the test code that triggers the crash)
similar to [5c82d91] in other fill/blit functions where len is signed and may go negative
similar to [5c82d91] in other fill/blit functions where len is signed and may go negative

Description draymond 2015-03-10 18:25:28 UTC
Created attachment 114208 [details]
(PDF file used in the test code that triggers the crash)

(this is using cairo 1.14.0 and poppler 0.32.0)

I've created a minimal test case that reliably reproduces the crash.  It 
crashes on both Windows and OS X.  However, on Windows it crashes at 
window size (1202, 931) while on OS X it crashes at window size 
(1207,932).  It also seems to be data related because the test PDF has 
two pages and it only crashes when rendering page 2.

The test PDF is attached.  Here is the test code:

#include <poppler.h>
#include <stdio.h>    // printf()

int main(int argc, char **argv)
{
    PopplerDocument *doc;
    PopplerPage *page;
    cairo_surface_t *surface;
    cairo_t *cr;
    double x_points, y_points;           // measured in points which are 
1/72 inch
    double pdf_width, pdf_height;        // measured in pixels
    double window_width, window_height;  // measured in pixels
    int    fit_width;
    double scale_factor;
    double x_padding;
    double y_padding;

#ifdef __APPLE__
    doc = 
poppler_document_new_from_file("file:///Users/draymond/crash/test.pdf", 
NULL, NULL);
#else
    doc = poppler_document_new_from_file("file:///C:/crash/test.pdf", 
NULL, NULL);
#endif
    page = poppler_document_get_page(doc, 1);  // does not crash 
rendering page 0

    for (window_width = 1200; window_width <= 1210; window_width++)
    {
       for (window_height = 930; window_height <= 940; window_height++)
       {
          printf("(%f, %f)\n", window_width, window_height);

          poppler_page_get_size(page, &x_points, &y_points); // 792, 612
          fit_width = (window_width / window_height) < (x_points / 
y_points);
          scale_factor = fit_width ? (window_width / x_points) : 
(window_height / y_points);
          pdf_width = x_points * scale_factor;
          pdf_height = y_points * scale_factor;
          x_padding = fit_width ? 0 : ((window_width - pdf_width) / 2);
          y_padding = fit_width ? ((window_height - pdf_height) / 2) : 0;

          surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 
window_width, window_height);
          cr = cairo_create(surface);

          cairo_translate(cr, x_padding, y_padding);
          cairo_scale(cr, scale_factor, scale_factor);
          poppler_page_render(page, cr);

          cairo_destroy(cr);
          cairo_surface_destroy(surface);
       }
    }

    printf("success\n");
    g_object_unref(page);
    g_object_unref(doc);
}
Comment 1 draymond 2015-03-10 18:26:45 UTC
Here is more GDB info showing the faulting instruction, the contents of 
the bad pointer, and the full backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x68fe680d in _fill_xrgb32_lerp_opaque_spans ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
(gdb) x/i $pc
=> 0x68fe680d <_fill_xrgb32_lerp_opaque_spans+281>: mov    (%eax),%ecx
(gdb) info registers
eax            0x30b5000        51073024
ecx            0x0      0
edx            0xfff57b52       -689326
ebx            0x165    357
esp            0x28d960 0x28d960
ebp            0x28d9c8 0x28d9c8
esi            0x194    404
edi            0x28da80 2677376
eip            0x68fe680d       0x68fe680d 
<_fill_xrgb32_lerp_opaque_spans+281>
eflags         0x10286  [ PF SF IF RF ]
cs             0x23     35
ss             0x2b     43
ds             0x2b     43
es             0x2b     43
fs             0x53     83
gs             0x2b     43
(gdb) bt
#0  0x68fe680d in _fill_xrgb32_lerp_opaque_spans ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#1  0x69038015 in blit_a8 () from C:\msys2\usr\local\bin\libcairo-2.dll
#2  0x69038593 in glitter_scan_converter_render ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#3  0x6903872e in _cairo_tor_scan_converter_generate ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#4  0x690279ae in composite_polygon ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#5  0x69027e5b in clip_and_composite_polygon ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#6  0x690281fc in _cairo_spans_compositor_stroke ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#7  0x68fd7386 in _cairo_compositor_stroke ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#8  0x68fecf54 in _cairo_image_surface_stroke ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#9  0x6902cb09 in _cairo_surface_stroke ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#10 0x68fdfdad in _cairo_gstate_stroke ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#11 0x68fda87c in _cairo_default_context_stroke ()
    from C:\msys2\usr\local\bin\libcairo-2.dll
#12 0x68fd0d04 in cairo_stroke () from C:\msys2\usr\local\bin\libcairo-2.dll
#13 0x6cc1b143 in CairoOutputDev::stroke (this=0x289e5d8, state=0x30e6ea8)
     at CairoOutputDev.cc:783
#14 0x008764ff in Gfx::opStroke (this=0x31079c0, args=0x28fa80, numArgs=0)
     at Gfx.cc:1856
#15 0x0086a599 in Gfx::execOp (this=this at entry=0x31079c0,
     cmd=cmd at entry=0x28fa58, args=args at entry=0x28fa80, 
numArgs=numArgs at entry=0)
     at Gfx.cc:904
#16 0x00872c3f in Gfx::go (this=this at entry=0x31079c0,
     topLevel=topLevel at entry=true) at Gfx.cc:763
#17 0x0087312e in Gfx::display (this=0x31079c0, obj=0x28fd80, topLevel=true)
     at Gfx.cc:729
#18 0x008b5849 in Page::displaySlice (this=0x289fda8, out=0x289e5d8, 
hDPI=72,
     vDPI=72, rotate=0, useMediaBox=false, crop=true, sliceX=-1, sliceY=-1,
     sliceW=-1, sliceH=-1, printing=false, abortCheckCbk=0x0,
     abortCheckCbkData=0x0, annotDisplayDecideCbk=0x0,
     annotDisplayDecideCbkData=0x0, copyXRef=false) at Page.cc:585
#19 0x6cc07e67 in _poppler_page_render (page=0x2867020, cairo=0x28d4fe8,
     printing=<optimized out>, print_flags=POPPLER_PRINT_DOCUMENT)
     at poppler-page.cc:362
#20 0x00401840 in main ()
Comment 2 draymond 2015-03-12 20:02:53 UTC
I have confirmed that the change submitted below suppresses the crash during my test case:

http://cgit.freedesktop.org/cairo/commit/?id=5c82d91a5e15d29b1489dcb413b24ee7fdf59934

This change was pulled into cairo 1.14.2.

However, the original author of that patch (ilia.softway@gmail.com) has expressed doubts that this change fully fixes the problem and he is now using an additional patch to cell_list_add_subspan() in cairo-tor_scan-converter.c.

Therefore, I think it is best to leave this bug open until the root cause is fully understood.
Comment 3 dmoppert 2016-07-25 01:32:27 UTC
Created attachment 125303 [details] [review]
similar to [5c82d91] in other fill/blit functions where len is signed and may go negative
Comment 4 dmoppert 2016-07-25 01:32:59 UTC
Created attachment 125304 [details] [review]
similar to [5c82d91] in other fill/blit functions where len is signed and may go negative
Comment 5 dmoppert 2016-07-25 01:35:29 UTC
Any news on the reporter's further work?

While reviewing this change for a backport I noticed a couple of other obvious cases of potentially the same flaw, where (len--) is used to bound a loop and len is signed int which may go negative.

Attached two patches dealing with these cases.
Comment 6 Bryce Harrington 2017-08-22 00:24:40 UTC
The backtrace you posted unfortunately is against a cairo built without symbols, so it's not clear where the crash within Cairo happened.  What Ilia might be referring to is that the fix is suitably defensive but doesn't address why the value is negative in the first place.  So, the patch probably did actually fix the crashing behavior, but there may also need to be some input value checking added somewhere.

I've opted to squash the two patches and land them, as they're also adding defensive programming checks.

I'll leave the bug open for now, if others wish to investigate why the invalid inputs are entering Cairo.

commit 63f14d4a8f155ebaaca63b49e7bacca55d681af5
Author:     Doran Moppert <dmoppert@redhat.com>
AuthorDate: Mon Jul 25 11:00:21 2016 +0930
Commit:     Bryce Harrington <bryce@osg.samsung.com>
CommitDate: Mon Aug 21 17:08:47 2017 -0700

    image: Check for negative len in fill/blit functions

    Applies the same fix as 5c82d91 to other potential negative len cases.

    Reviewed-by: Bryce Harrington <bryce@osg.samsung.com>
Comment 7 GitLab Migration User 2018-08-25 13:43:14 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/157.

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.