[Poppler-bugs] [Bug 24575] evince crashed with SIGSEGV in CairoOutputDev::restoreState()

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=24575





--- Comment #11 from David Benjamin <davidben at mit.edu>  2009-11-27 12:51:53 PST ---
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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.