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

[Matthias Hopf]

On Mar 24, 10 18:28:07 +0100, Soeren Sandmann wrote:
> Please note that while pixman is not as strict as the X server in who
> can push to the repository, it is not a complete free-for-all.
> Committing small, obvious patches that fixes typos or oversights is
> fine, but don't commit non-obvious stuff without sending it to the
> mailing list and giving people time to comment on it.

Ok, just the comment from Jonathan indicated to me that I should commit.

> When we are this close to stable release, we need to be even more
> careful about what goes in. Patches committed between now and next
> week will likely ship without no testing at all.

It definitely did help here. So there's at least minimal testing :)

> This patch in particular, I don't think shold ship with no testing at
> all. So please revert it, and we can consider it again for 0.19.x.

I'm fine with that if you consider it problematic. Given that the
situation it changes should actually not occur at all, I doubt there's
any fallout, though.

> - Why are you seeing an assert in the first place? 
> 
>   The asserts have been disabled since 0.16.4/0.17.6 because pixman
>   shouldn't take down the X server with asserts for things that would
>   otherwise be harmless.

Apparently we're still using 0.16.0 in SLE11SP1. So that explains.
I double checked that there were no related changes in the region code
before fixing.

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.

>   If you are using an older version than those in SLES, then you might
>   want to either upgrade to 0.16.4, or backport

No chance. We are version and feature freeze for quite some time now.
Time cycles are much different in enterprise products :-/
Especially in SLE11SP1, as changes to SLE11 should be as minimal as
possible (and SLE11 is over a year old).

>        ec6de472d042bec05aaa53f9d14bfbe23edbe01e

That sounds more realistic. However, we don't have any other issues with
assert()s, and there is a slim chance that this backport introduces
additional regressions (asserts with side effects etc.).

> - How does the degenerate region get created?

Unfortunately I don't know the offending Xserver code by heart.
It probably has to be reviewed, I guess there are sleeping dragons there.

> - The patch does not look wrong to me - a region with empty extents
>   should probably be considered empty rather than degenerate. However,
>   unless this issue is truly causing crashes, changing it is the kind

It is definitely causing crashes here, reproducibly, but only on very
rare occasions (of which we run into one during installation :o] )

>   candidate and a stable release. Did you carefully review all the
>   code to make sure it always uses PIXMAN_NIL to check for empty
>   regions?

No, but that shouldn't change behavior. Unless a single code snippet
uses both PIXMAN_NIL and handcoded tests at the same time.

> - Finally, we need much more detail in commit messages than "Fixes
>   Novell bug xxxxxx".

Understood. I thought the code change to be trivial in itself.

>   Is that even a public bug? I couldn't figure out how to look at
>   it. I did not create a Novell Login though.

Unfortunately, no. Not that I did notice before, oops. You tend to miss
those things if you're working with credentials set all the time...
It doesn't contain much more information than the backtrace I published,
though.

Matthias

-- 
Matthias Hopf <mhopf at suse.de>      __        __   __
Maxfeldstr. 5 / 90409 Nuernberg   (_   | |  (_   |__          mat at mshopf.de
Phone +49-911-74053-715           __)  |_|  __)  |__  R & D   www.mshopf.de