Comment 17 for bug 430887

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.