evince crashed with SIGSEGV in CairoOutputDev::restoreState()

Bug #430887 reported by Flávio Etrusco
52
This bug affects 9 people
Affects Status Importance Assigned to Milestone
Poppler
Fix Released
Medium
poppler (Ubuntu)
Fix Released
Medium
Ubuntu Desktop Bugs

Bug Description

Binary package hint: evince

Evince crashes while loading the attached document (also available for now at http://www.orm.com.br/projetos/tvliberal/pdf/LISTADEPRE%C3%87OS_abrilsemtembro.pdf).
If the thumbnail pane is enabled, evince crashes after displaying (apparently corrupted) the 3rd page; if the pane is closed before opening the file, evince only crashes if I scroll down and try to display the 3rd page or any later.
Even in the latter case and before scrolling down, the error output already contains plenty of errors. IOW while seemingly decoding the first page.

ProblemType: Crash
Architecture: i386
Date: Wed Sep 16 13:20:18 2009
DistroRelease: Ubuntu 9.10
ExecutablePath: /usr/bin/evince
Package: evince 2.27.90-0ubuntu8
ProcCmdline: root=UUID=e81fb5c3-6a49-49f4-96ad-eaafcb62518d ro quiet splash usbcore.autosuspend=1
ProcEnviron:
 PATH=(custom, user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
ProcVersionSignature: Ubuntu 2.6.31-10.34-generic
SegvAnalysis:
 Segfault happened at: 0x18683d <_ZN14CairoOutputDev12restoreStateEP8GfxState+157>: mov (%edi),%eax
 PC (0x0018683d) ok
 source "(%edi)" (0x00000000) not located in a known VMA region (needed readable region)!
 destination "%eax" ok
SegvReason: reading NULL VMA
Signal: 11
SourcePackage: evince
StacktraceTop:
 CairoOutputDev::restoreState(GfxState*) ()
 Gfx::restoreState() () from /usr/lib/libpoppler.so.5
 Gfx::opRestore(Object*, int) ()
 Gfx::execOp(Object*, Object*, int) ()
 Gfx::go(int) () from /usr/lib/libpoppler.so.5
Title: evince crashed with SIGSEGV in CairoOutputDev::restoreState()
Uname: Linux 2.6.31-10-generic i686
UserGroups: adm admin audio cdrom dialout dip fuse lpadmin netdev plugdev sambashare vboxusers video

Related branches

Revision history for this message
Flávio Etrusco (etrusco) wrote :
affects: evince (Ubuntu) → poppler (Ubuntu)
Revision history for this message
Apport retracing service (apport) wrote : Stacktrace.txt (retraced)

StacktraceTop:CairoOutputDev::restoreState (this=0xa30cbf8, state=0xb65046a0)
Gfx::restoreState (this=0xb6504540) at Gfx.cc:4716
Gfx::opRestore (this=0xb6504540, args=0xb6e58dc4, numArgs=0)
Gfx::execOp (this=0xb6504540, cmd=0xb6e58f64,
Gfx::go (this=0xb6504540, topLevel=1) at Gfx.cc:661

Revision history for this message
Apport retracing service (apport) wrote : ThreadStacktrace.txt (retraced)
Changed in poppler (Ubuntu):
importance: Undecided → Medium
tags: removed: need-i386-retrace
Revision history for this message
In , Pedro Villavicencio (pedro) wrote :
Download full text (15.0 KiB)

this report has been filed here:

https://bugs.edge.launchpad.net/ubuntu/+source/poppler/+bug/430887

"Evince crashes while loading the attached document (also available for now at http://www.orm.com.br/projetos/tvliberal/pdf/LISTADEPRE%C3%87OS_abrilsemtembro.pdf).
If the thumbnail pane is enabled, evince crashes after displaying (apparently corrupted) the 3rd page; if the pane is closed before opening the file, evince only crashes if I scroll down and try to display the 3rd page or any later.
Even in the latter case and before scrolling down, the error output already contains plenty of errors. IOW while seemingly decoding the first page."

".
Thread 3 (process 3547):
#0 0x0027d422 in __kernel_vsyscall ()
No symbol table info available.
#1 0x00b3cba6 in *__GI___poll (fds=0xbd1ff4, nfds=9, timeout=9)
    at ../sysdeps/unix/sysv/linux/poll.c:87
 resultvar = <value optimized out>
 oldtype = 0
 result = <value optimized out>
#2 0x00f2c52b in IA__g_poll (fds=0xa096e50, nfds=9, timeout=9)
    at /build/buildd/glib2.0-2.21.6/glib/gpoll.c:127
No locals.
#3 0x00f1f5fb in g_main_context_iterate (context=0xa0ac058,
    block=<value optimized out>, dispatch=1, self=0xa094050)
    at /build/buildd/glib2.0-2.21.6/glib/gmain.c:2904
 max_priority = 2147483647
 timeout = 9
 some_ready = <value optimized out>
 nfds = 9
 allocated_nfds = <value optimized out>
 fds = <value optimized out>
 __PRETTY_FUNCTION__ = "g_main_context_iterate"
#4 0x00f1fc2f in IA__g_main_loop_run (loop=0xa096220)
    at /build/buildd/glib2.0-2.21.6/glib/gmain.c:2799
 self = (GThread *) 0xa094050
 __PRETTY_FUNCTION__ = "IA__g_main_loop_run"
#5 0x003fc6f9 in IA__gtk_main ()
    at /build/buildd/gtk+2.0-2.17.11/gtk/gtkmain.c:1205
 tmp_list = (GList *) 0xa0a14e4
 functions = (GList *) 0x0
 init = (GtkInitFunction *) 0x0
 loop = (GMainLoop *) 0xa096220
#6 0x08080aa2 in main (argc=1, argv=0xbfc3d654) at main.c:497
 context = <value optimized out>
 args = (GHashTable *) 0xa0e16f0
 error = (GError *) 0x0
.
Thread 2 (process 3549):
#0 0x0027d422 in __kernel_vsyscall ()
No symbol table info available.
#1 0x00144142 in pthread_cond_timedwait@@GLIBC_2.3.2 ()
   from /lib/tls/i686/cmov/libpthread.so.0
No symbol table info available.
#2 0x0088315e in g_cond_timed_wait_posix_impl (cond=0xfffffdfc,
    entered_mutex=0x1, abs_time=0xb63bf288)
    at /build/buildd/glib2.0-2.21.6/gthread/gthread-posix.c:242
 result = <value optimized out>
 end_time = {tv_sec = 1253118030, tv_nsec = 894223000}
 timed_out = <value optimized out>
 __PRETTY_FUNCTION__ = "g_cond_timed_wait_posix_impl"
#3 0x00ef6cac in g_async_queue_pop_intern_unlocked (queue=0xa29a0c0,
    try=<value optimized out>, end_time=0xb63bf288)
    at /build/buildd/glib2.0-2.21.6/glib/gasyncqueue.c:365
 retval = <value optimized out>
 __PRETTY_FUNCTION__ = "g_async_queue_pop_intern_unlocked"
#4 0x00ef6db1 in IA__g_async_queue_timed_pop (queue=0xa29a0c0,
    end_time=0xb63bf288)
    at /build/buildd/glib2.0-2.21.6/glib/gasyncqueue.c:491
 retval = <value optimized out>
 __PRETTY_FUNCTION__ = "IA__g_async_queue_timed_pop"
#5 0x00f47b1e in g_thread_pool_thread_proxy (data=0xa2e16f0)
    at /build/buildd/glib2.0-2.21.6/glib/gthreadpool.c:1...

visibility: private → public
Revision history for this message
Pedro Villavicencio (pedro) wrote :

confirming, same crash with the document you pointed there. will send upstream , thanks.

Changed in poppler (Ubuntu):
status: New → Confirmed
assignee: nobody → Ubuntu Desktop Bugs (desktop-bugs)
Revision history for this message
Pedro Villavicencio (pedro) wrote :

Thank you for your bug report. This bug has been reported to the developers of the software. You can track it and make comments at:
 https://bugs.freedesktop.org/show_bug.cgi?id=24575

Changed in poppler (Ubuntu):
status: Confirmed → Triaged
Changed in poppler:
status: Unknown → Confirmed
Revision history for this message
In , David Benjamin (davidben) wrote :

Created an attachment (id=31363)
Do not restoreState on empty stack

This PDF is horrendously malformed. As far as I can tell, several of the pages have command streams missing spaces and who knows what else. Even acroread will display an error. That said, acroread does not crash and does a much better job rendering it than we do.

The confused command stream results in a restoreState without the right number of saveStates. The above patch causes that to result in a warning.

We probably want to try being lenient about parsing the command stream to allow one to omit the spaces before/after commands? I suspect this is what acroread is doing. This may be tricky as some commands (d0 and d1) end in numbers.

Revision history for this message
In , Albert Astals Cid (aacid) wrote :

Yes that'd would be the end goal, we already do that this space thing when parsing obj number, see XRef.cc line 963

About this patch, i'd prefer to move it to the CairoOutputDev, the other *OutputDev already do this check, and in a regular project i'd suggest "upstreaming" the checks but here i prefer not to touch existing code to ease future merges with xpdf code (if he ever does a new release) so moving to CairoOutputDev is as good for fixing the bug and eases a bit the merging so if you agree Carlos or I can commit the fix there.

Revision history for this message
In , Carlos Garcia Campos (carlosgc) wrote :

Yes, go ahead.

Revision history for this message
In , Albert Astals Cid (aacid) wrote :

Pushed, should we open a different bug for the rendering thing?

Revision history for this message
In , David Benjamin (davidben) wrote :

It might be worth checking the PDF more carefully if it's really just a spaces thing. I see a block

442.5542 466.4069 l
442.4207 466.2510 442.2421 466.0923 442.0182 465.9284 c
441.7940 46.63657 441.5888 465.6430 441.4008 465.5602 c
441.4008 466.2391 l
441.7194 466.4217 441.9995 466.6445 442.2436 466.9070 c
442.4862 467.1711 442.6580 46
h2.71 442.7566 46
h6777 c
443.1314 46
h6777 l
h
h
444.6473 466.9030 m

which only makes sense as far as argument counts go if we just ignore the h2.71 and h6777 tokens. A first attempt at quickly implementing something like that didn't seem to make the PDF render right. Though, I could have decompressed it wrong --- what I did was rather hacky. Is there a tool somewhere that will decode all the streams of a PDF so that one may easily view the command streams in plain text?

Revision history for this message
In , Albert Astals Cid (aacid) wrote :

you can try podofobrowser, i'm working on a "uncompress" tool based on poppler since we have all the bits, just never found the time :D

Revision history for this message
In , Carlos Garcia Campos (carlosgc) wrote :

Now, I see the error message with any PDF document:

"Error: restoreState on an empty state stack"

so, I'm not sure the patch is right.

Revision history for this message
In , David Benjamin (davidben) wrote :

It looks like that got introduced when porting the patch down to CairoOutputDev. I get this error on a random PDF with master, but not the original patch.

I think it's because we call state->restore() before the output device sees anything.

 void Gfx::restoreState() {
   state = state->restore();
   out->restoreState(state);
 }

Revision history for this message
In , Carlos Garcia Campos (carlosgc) wrote :

What about just something like this?

- mask = ms->mask;
- maskStack = ms->next;
- delete ms;
+ if (ms) {
+ mask = ms->mask;
+ maskStack = ms->next;
+ delete ms;
+ }

that's what we already do with other stacks.

Revision history for this message
In , Carlos Garcia Campos (carlosgc) wrote :

Fianlly fixed it in git master and poppler-0.12 branch.

Revision history for this message
In , David Benjamin (davidben) wrote :

I'm... not convinced this actually works. I think the patch as it is still can cause a crash.

Say we're at a point where the state stack is empty, but we have set a mask. So out->mask is not null. Then we call Gfx::restoreState. In the current code, we call CairoOutputDev::restoreState which reaches this code:

  MaskStack* ms = maskStack;
  if (mask)
    cairo_pattern_destroy(mask);

  if (ms) {
    mask = ms->mask;
    maskStack = ms->next;
    delete ms;
  }

This frees the current mask, but fails to replace it with a different one because ms is NULL. Future uses of mask will then access freed memory and cause problems.

We could instead write

  if (mask) {
    cairo_pattern_destroy(mask);
    mask = NULL;
  }

which wouldn't crash, but then an erroneous restoreState would clear your mask --- something I'm not convinced is desirable.

The main problem being that, if we do a Gfx::restoreState on an empty stack, it really should be a no-op[1]. However, without the original patch of checking in the generic layer, we still call an OutputDev::restoreState, which is then responsible for enforcing this. This is messy, and, more importantly, actually difficult to detect without extra state. In both the case of restoring to an empty stack and restoring to a 1-element stack, OutputDev::restoreState receives the same GfxState object and has no way of distinguishing between the two without maintaining its own state, which seems rather poor.

In the case of CairoOutputDev, on any restoreState, we forcibly pop the mask, cairo context and cairo_shape context[2].

I think it's worth adding the check to the output-independent part even if future merges with xpdf may be somewhat of a nuisance. Especially if it turns out acroread does something fancy on empty restoreState[1], we'll need to add code there anyway. Also, it looks like the last xpdf release was almost 3 years ago.

[1] Or maybe a restore to default state? Might be worth seeing exactly what acroread does here.

[2] Actually, I think the logic here doesn't work if cairo_shape is introduced/removed on anything other than the bottom-most stack. I'll try to come up with a test case and file a bug if it indeed doesn't work.

Changed in poppler:
status: Confirmed → Fix Released
Changed in poppler (Ubuntu):
status: Triaged → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package poppler - 0.12.3-0ubuntu1

---------------
poppler (0.12.3-0ubuntu1) lucid; urgency=low

  * New upstream version:
    - [Cairo backend] Do not crash on malformed files. Bug #24575 (lp: #430887)
    - [Cairo backend] Fix crash in some documents. GNOME bug #603934 (
      lp: #436197)
  * debian/patches/01_git_change_fix_matrix_use.patch:
    - git change to fix a wrong matrix use leading to rendering issues
      (lp: #252552)
 -- Sebastien Bacher <email address hidden> Fri, 29 Jan 2010 12:17:57 +0100

Changed in poppler (Ubuntu):
status: Fix Committed → Fix Released
Changed in poppler:
importance: Unknown → Medium
Changed in poppler:
importance: Medium → Unknown
Changed in poppler:
importance: Unknown → Medium
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.