Bug 97870 - Crash when calling cmsGetColorSpace
Summary: Crash when calling cmsGetColorSpace
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
: 98678 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-09-20 09:04 UTC by Marek Kasik
Modified: 2016-12-15 21:05 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
reproducer (704.18 KB, application/pdf)
2016-09-20 09:04 UTC, Marek Kasik
Details
Don't crash when calling cmsGetColorSpace() (1.60 KB, patch)
2016-09-20 09:05 UTC, Marek Kasik
Details | Splinter Review
Don't crash when calling cmsGetColorSpace() (1.05 KB, patch)
2016-09-21 10:36 UTC, Marek Kasik
Details | Splinter Review

Description Marek Kasik 2016-09-20 09:04:41 UTC
Created attachment 126644 [details]
reproducer

Evince sometimes crashes in cmsGetColorSpace() because it gets NULL as an argument. I was able to reproduce this with the attached file. The crash happens only in evince's recent view mode so the file has to be opened before that.

Steps to reproduce:
1) run "evince postscript_un_linguaggio_per_la_composizione_finale.pdf"
2) close evince
3) run evince

Backtrace:
#0  0x00007fffe007c210 in cmsGetColorSpace (hProfile=0x0) at cmsio0.c:952
#1  0x00007fffc98b69be in GfxICCBasedColorSpace::parse(Array*, OutputDev*, GfxState*, int) (arr=0x7fffb82255b0, out=0x0, state=0x0, recursion=1) at GfxState.cc:2055
#2  0x00007fffc98b0506 in GfxColorSpace::parse(GfxResources*, Object*, OutputDev*, GfxState*, int) (res=0x0, csObj=0x7fffca7fb7a0, out=0x0, state=0x0, recursion=1) at GfxState.cc:406
#3  0x00007fffc98b906f in GfxIndexedColorSpace::parse(GfxResources*, Array*, OutputDev*, GfxState*, int) (res=0x0, arr=0x7fffb81ebea0, out=0x0, state=0x0, recursion=0) at GfxState.cc:2590
#4  0x00007fffc98b0588 in GfxColorSpace::parse(GfxResources*, Object*, OutputDev*, GfxState*, int) (res=0x0, csObj=0x7fffca7fb990, out=0x0, state=0x0, recursion=0) at GfxState.cc:408
#5  0x00007fffc98f2042 in Page::loadThumb(unsigned char**, int*, int*, int*) (this=0x7fffb80625f0, data_out=0x7fffca7fba58, width_out=0x7fffca7fba54, height_out=0x7fffca7fba50, rowstride_out=0x7fffca7fba4c) at Page.cc:691
#6  0x00007fffe02f85b0 in poppler_page_get_thumbnail(PopplerPage*) (page=0x7fffb8060040) at poppler-page.cc:491
#7  0x00007fffe05450bf in pdf_document_get_thumbnail_surface(EvDocument*, EvRenderContext*) (document=0x7fffd8003730, rc=0xc61940) at ev-poppler.cc:538
#8  0x00007ffff7bacced in ev_document_get_thumbnail_surface (document=0x7fffd8003730, rc=0xc61940) at ev-document.c:777
#9  0x00007ffff7948889 in ev_job_thumbnail_run (job=0xe58980) at ev-jobs.c:885
#10 0x00007ffff79468f4 in ev_job_run (job=0xe58980) at ev-jobs.c:216
#11 0x00007ffff794b92b in ev_job_thread (job=0xe58980) at ev-job-scheduler.c:184
#12 0x00007ffff794ba17 in ev_job_thread_proxy (data=0x0) at ev-job-scheduler.c:217
#13 0x00007ffff4238ae3 in g_thread_proxy () at /usr/lib64/libglib-2.0.so.0
#14 0x00007ffff3a916ca in start_thread (arg=0x7fffca7fc700) at pthread_create.c:333
#15 0x00007ffff37cbf6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105


Here are downstream reports for this crash:
https://retrace.fedoraproject.org/faf/reports/1219994/
https://bugzilla.redhat.com/show_bug.cgi?id=1363669
https://bugzilla.redhat.com/show_bug.cgi?id=1293445
https://retrace.fedoraproject.org/faf/reports/896435/
Comment 1 Marek Kasik 2016-09-20 09:05:53 UTC
Created attachment 126645 [details] [review]
Don't crash when calling cmsGetColorSpace()

This patch fixes the problem for me.
Comment 2 Carlos Garcia Campos 2016-09-20 16:21:55 UTC
(In reply to Marek Kasik from comment #1)
> Created attachment 126645 [details] [review] [review]
> Don't crash when calling cmsGetColorSpace()
> 
> This patch fixes the problem for me.

Is that the only place where all those global variables could be used uninitialized? GfxColorSpace::setupColorProfiles() is already protected to ensure it only happens once even if called multiple times, so I think it would be simpler to call it unconditionally before trying to use RGBProfile. But I would make this generic, the current use of all those global vars is very weak in my opinion. I would make a global singleton to contain those variables or something similar.
Comment 3 Marek Kasik 2016-09-21 10:36:09 UTC
Created attachment 126698 [details] [review]
Don't crash when calling cmsGetColorSpace()

(In reply to Carlos Garcia Campos from comment #2)
> (In reply to Marek Kasik from comment #1)
> > Created attachment 126645 [details] [review] [review] [review]
> > Don't crash when calling cmsGetColorSpace()
> > 
> > This patch fixes the problem for me.
> 
> Is that the only place where all those global variables could be used
> uninitialized? GfxColorSpace::setupColorProfiles() is already protected to
> ensure it only happens once even if called multiple times, so I think it
> would be simpler to call it unconditionally before trying to use RGBProfile.

Some of those variables can be used uninitialized via getters like:
  GfxColorSpace::getRGBProfile()
  GfxColorSpace::getDisplayProfile()

The XYZ2DisplayTransform is also used in some ::parse() methods but these usages are guarded by consequent NULL checks.

I've changed the patch to call the setup of color profiles unconditionally.
Comment 4 Vlad Orlov 2016-10-23 15:17:31 UTC
Thanks Marek, I confirm this fixes the issue with atril-thumbnailer crashing (Atril is a fork of Evince). I've patched poppler 0.41.0 in Ubuntu 16.04 and the crash is now gone.

For reference - Ubuntu bug report about that:
https://bugs.launchpad.net/bugs/1635812
Comment 5 Albert Astals Cid 2016-11-10 23:00:41 UTC
*** Bug 98678 has been marked as a duplicate of this bug. ***
Comment 6 Michael Natterer 2016-11-10 23:32:17 UTC
Is theree a poppler release which contains the fix?
Comment 7 Albert Astals Cid 2016-11-10 23:41:37 UTC
The bug isn't closed, so not, it hasn't even been commited to the git repo.

Personally i'm not convinced by the patch but since Carlos took lead on the review i'm leaving it to him to decide
Comment 8 Carlos Garcia Campos 2016-11-12 08:04:03 UTC
(In reply to Albert Astals Cid from comment #7)
> The bug isn't closed, so not, it hasn't even been commited to the git repo.
> 
> Personally i'm not convinced by the patch but since Carlos took lead on the
> review i'm leaving it to him to decide

What are your concerns?
Comment 9 Albert Astals Cid 2016-11-14 22:07:11 UTC
(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Albert Astals Cid from comment #7)
> > The bug isn't closed, so not, it hasn't even been commited to the git repo.
> > 
> > Personally i'm not convinced by the patch but since Carlos took lead on the
> > review i'm leaving it to him to decide
> 
> What are your concerns?

I feel the solution is a bit too much of "using a cannon to kill flies" from the architectural point of view (since from the performance point of view probably will be fine)
Comment 10 Michael Natterer 2016-12-11 18:53:17 UTC
Any news here? This is a blocker for GIMP because poppler simply crashes
without any way for us to prevent that. We'd like to depend on a poppler
version that has this fixed :)


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.