Bug 6827

Summary: [patch] crash in fb
Product: xorg Reporter: Radek Doulik <rodo>
Component: * OtherAssignee: Adam Jackson <ajax>
Status: RESOLVED FIXED QA Contact:
Severity: critical    
Priority: high CC: alexdeucher, d.jacobfeuerborn, mat
Version: gitKeywords: patch
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 5041    
Attachments:
Description Flags
proposed patch
none
Proposed updated patch none

Description Radek Doulik 2006-05-03 21:24:28 UTC
There's a problem with fbFetchTransformed function, which can access data
outside the pixmap buffer. It frequently leads to Xserver crash. (the offset in
device memory is wrong calculated only for offscreen pixmaps)

Proposed patch fixes offset calculation. It also introduces 2 new temporary
variables dx, dy to avoid frequent double-dereferencing.
Comment 1 Radek Doulik 2006-05-03 21:25:07 UTC
Created attachment 5554 [details] [review]
proposed patch
Comment 2 Adam Jackson 2006-05-09 10:08:56 UTC
think this is the solution to bug #6704, currently testing.

radek, do you happen to have a testcase for this?  would be a good thing to add
to rendercheck if so.
Comment 3 Vladimir Vukicevic 2006-05-09 10:18:54 UTC
Mozilla 1.9 (trunk builds, what will become Firefox 3.0) is hitting this; see
https://bugzilla.mozilla.org/show_bug.cgi?id=334951 .
Comment 4 Radek Doulik 2006-05-09 19:43:38 UTC
I don't have a rendercheck test for it. I was using simple impress presentation
which exposed that problem - rectangle are with transparency set to 50%.

rendercheck test might be probably easy to add, every operation which requires
fbFetchTransformed should expose that bug (the picture pixmap must be offscreen
though)
Comment 5 Matthias Hopf 2006-05-10 00:18:28 UTC
The full story to this patch can be viewed in
https://bugzilla.novell.com/show_bug.cgi?id=152730

Be aware, this bug is *extremely* long with more than 100 comments.
Comment 6 Adam Jackson 2006-05-10 00:59:54 UTC
(In reply to comment #5)
> The full story to this patch can be viewed in
> https://bugzilla.novell.com/show_bug.cgi?id=152730
> 
> Be aware, this bug is *extremely* long with more than 100 comments.

Wow, nice find.  radek++

I passed this patch by keithp and he didn't see anything obviously wrong with
it.  I'll land this for RC2 if all goes well on #6704.
Comment 7 Matthias Hopf 2006-05-10 02:05:32 UTC
I'm currently reviewing the patch, and there are a couple of issues I have to
think about:

- Radek has been using pDrawable->x/y to retransform the coordinates. On the
other hand, the offsets created by fbGetDrawable seem to be better suited for
this job (pixmap offset with eventually screen offset).
- These offsets are first added to x/y, and now pDrawable->x/y is subtracted.
This should level out.
- AFAICS pict->pComositeClip->extens should *not* be changed. It might have no
side effects, but I guess the save way is to not change the values. I'll test
this in a second step.
- I think Radek missed one box change.

I'm currently rewriting and testing this, if it works out to fix the bug as
well, I'll post here to you and Keith to comment which one is the right one.
Maybe even a mixture :)
Comment 8 Matthias Hopf 2006-05-10 04:56:37 UTC
(In reply to comment #7)
> - I think Radek missed one box change.

Update:
Right now I believe that several of the box constructs are dead code.

My adapted patch (with the idea about xoff/yoff) clearly showed that I didn't
understand the offset correctly as it doesn't work as well. I will have to check
this further.

Please note also, that this fixes the issue for Xorg 7.0, but for current CVS it
only improves the situation, but it doesn't completely fix. There are clearly
more render bugs triggered in CVS. The background in impress is still black
instead of gray, so there must be another bug with SolidColors.

I also ask myself whether fbFetchSourcePict is affected. It doesn't do anything
with offsets. Ajax, is fbFetchSourcePict only called on non-accelerated or
non-offscreen pixmaps?
Comment 9 Radek Doulik 2006-05-10 19:51:38 UTC
> Please note also, that this fixes the issue for Xorg 7.0, but for current CVS it
> only improves the situation, but it doesn't completely fix. There are clearly
> more render bugs triggered in CVS. The background in impress is still black
> instead of gray, so there must be another bug with SolidColors.

I was preparing the patch with cvs version, week or two ago. What presentation
shows the black/gray problem? Are you sure it doesn't work correctly with my
patch? (I know how easy it is to break that function when playing with offsets ;-)

Note that I was also using your previous patch for pPicture->format, without it
I experienced errors in rendercheck.

Comment 10 Matthias Hopf 2006-05-10 20:25:55 UTC
My previous patch is already in CVS (had been found independently by - err, I
don't remember :-] ).

Both presentations in the suse bugzilla show the image prefectly with your
patch, but the background is just black (not white). On the second slide of the
more complex example the background image is not shown at all.

This is all only with current CVS. In Xorg 7.0 everything works fine, so
presumably another bug shows up that had been obscured before.
Comment 11 Radek Doulik 2006-05-10 20:33:22 UTC
> Both presentations in the suse bugzilla show the image prefectly with your
> patch, but the background is just black (not white). On the second slide of the
> more complex example the background image is not shown at all.

Interesting, I am experiencing something similar with Xgl. Might it be that it
is a different bug? Does it happen without patch for this bug as well?
Comment 12 Matthias Hopf 2006-05-10 20:38:32 UTC
Yes, and it appearantly wasn't connected to the additional patch I'm working on.
I will update my whole Xorg tree now and try again.
Comment 13 Dennis Jacobfeuerborn 2006-05-10 23:52:23 UTC
I'm the reporter of both fdo bug #6704 and the mozilla bug mentioned above. The
following page still does not display properly which might be related to all of
this: http://sinfest.net/d/20060429.html

What is interesting about this is that the image gets displayed fine as it loads
progressively but as soon as the loading is finished all or a part (from a
random row all the way down) of the image turns white.

The good news is that Xorg hasn't crashed for me since I installed the RPMs
linked to in bug #6704 so this patch definitely improves the situation.
Comment 14 Matthias Hopf 2006-05-11 20:57:07 UTC
Created attachment 5602 [details] [review]
Proposed updated patch

This is a trivially updated patch. It nukes some unused boxes and does not
change the extens boxes. I'm leaving some common subexpression eliminations to
the compiler.

Otherwise this patch behave equivalently (verified), on CVS and 7.0.
On 7.0 all issues are fixed.
On CVS background + background image are broken, but overlay images are ok now.
Comment 15 Adam Jackson 2006-05-16 04:16:49 UTC
*** Bug 6704 has been marked as a duplicate of this bug. ***
Comment 16 Adam Jackson 2006-05-19 00:41:00 UTC
I've applied this to 1.1 branch and HEAD, as being clearly better than what we
had before.  Thanks!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.