[Pixman] [PATCH] Fix server crash in pixman (to be discussed)

[Soeren Sandmann]

Matthias Hopf <mhopf at suse.de> writes:

> On Mar 24, 10 19:19:15 +0100, Soeren Sandmann wrote:
> > > However, what happens if the code would have been compiled with -NDEBUG?
> > > Is the code path stable with empty regions? If it is, it can be argued
> > > that the patch is not necessary, but it could also be argued that the
> > > assert() shouldn't have been there in the first place.
> > 
> > Who knows? Who knows if it's stable even *with* the patch? That's why
> > I don't want it in for 0.18.0.
> 
> Right. I just want to indicate that just disabling the assert()s
> typically is no "solution" for issues like this.

Here are some bugs that have caused asserts:

- The X server constructed a region directly from a list of rectangles
  that wasn't XY banded. This eventually caused asserts.

- The X server directly initialized the extents rectangle to an empty
  rectangle. (That's at least what I think is going on with your bug
  here).

- 16 bit integer overflows from a very large CopyArea caused the
  region to look strange which caused asserts.

The first one was fixed in the X server, and the second one could
be. The third one is difficult to do anything about because we really
do need regions with unsigned 16 bit extents. Relying on signed
integer overflow, which is undefined in C, is pretty dubious.

The solution to the third one is to simply move to 32 bit
regions. Things that should happen:

- X server should be moved to 32 bit pixman regions

- All the region macros and miRegion should go away

- In particular, the X server should stop calling
  pixman_region_set_static_pointers(). (This function only exists to
  preserve the driver ABI)

- pixman gets a new ABI in which the 16 bit regions don't exist
  anymore, and in which the names of the fields are changed.

- X server is made to compile against the new ABI.

Then asserts could be reenabled, but even then, crashing the X server
is not something to be done lightly.


Soren