bugGnash - The GNU Flash player - Bugs: bug #23624, segfault/crashes resizing w/ GTK...

 
 

bug #23624: segfault/crashes resizing w/ GTK and MIT-SHM enabled

Submitted by:  Bastiaan Jacques <bjacques>
Submitted on:  Wed 18 Jun 2008 06:51:43 AM UTC  
 
Category: gui-gtkSeverity: 6 - Security
Release: NoneStatus: Fixed
Privacy: PublicAssigned to: Bastiaan Jacques <bjacques>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 28 Nov 2008 09:36:45 AM UTC, comment #14:

Isn't shm now enabled when it's available? If so, this bug seems fixed.

Benjamin Wolsey <bwy>
Project Member
Fri 10 Oct 2008 04:55:56 PM UTC, comment #13:

Status: not fixed, but MIT-SHM is not enabled by default. So not blocking.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Fri 10 Oct 2008 12:43:10 PM UTC, comment #12:

Status of this bug?

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Tue 15 Jul 2008 08:44:20 PM UTC, comment #11:

The segfault only triggers sometimes because an X window is drawn into and this should never segfault, even if you use the wrong sizes. However, when you switch to MIT-SHM a buffer is rendered into, which is much more likely to segfault.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Tue 15 Jul 2008 08:39:34 PM UTC, comment #10:

You're right - as I said my last comment, this doesn't fix the original bug. It was rather the cause of segfaults I at first thought might be related to the original bug.

I've been testing without the +1 (only with agg) and haven't noticed any rendering problems (apart from the unrelated bad one in bug #23846), so I'm not sure presuming it's needed is a good idea. The testsuite is fine, and it's certainly wrong when it enlarges the rectangle to go outside the buffer...

Anyway, the point is that, although the original analysis seems like it makes sense, I haven't yet produced any segfault from it. I think I'll look at the startStopped problem, as it has obvious symptoms and approaching that might suggest a way of fixing the initialization event order.

Benjamin Wolsey <bwy>
Project Member
Tue 15 Jul 2008 07:53:33 PM UTC, comment #9:

I believe the +1 thing was added by the original author to compensate for potential anti-aliasing. (I don't know if it's still needed, but I presume it is.)

That said, even if your commit fixes the crash for you, it does not fix the bug described in the original report of this bug.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Tue 15 Jul 2008 10:04:27 AM UTC, comment #8:

I've committed the fix to this last part as adding 1 seems incorrect. Haven't done anything on the original bug yet.

Benjamin Wolsey <bwy>
Project Member
Tue 15 Jul 2008 08:24:55 AM UTC, comment #7:

This is the immediate cause of a resize segfault for me when running the SWF file #16119.

From GtkAggGlue::render:
9679:47992769588144] 09:54:50 DEBUG: minx: 913, miny: 152, copy width: 3, copy height: 2, stride: 2745
9679:47992769588144] 09:54:50 DEBUG: offscreenbuf size: 420000
9679:47992769588144] 09:54:50 DEBUG: From: 419979

Copy height is always 2 when it segfaults. In this case, the allocated size was 913 x 153, so the updated rectangle was on the very bottom line. As the copy height is 2, we read outside the buffer.

The copy height is 2 because of this code, which always adds 1 to a non-full-screen invalidated rectangle:

size_t copy_width = std::min(_width * (_bpp/8), maxx - minx + 1);
size_t copy_height = std::min(_height, maxy - miny + 1);

Why does it do that? It is evidently wrong in this particular case, but I haven't explored enough to see whether it's the invalidated rectangle itself that's wrong (possibly caused by a resize) or whether we just shouldn't add 1. Dropping the 1 appears to fix segfaults in this SWF, and I haven't yet noticed any bad side effects.

Benjamin Wolsey <bwy>
Project Member
Sat 12 Jul 2008 12:37:24 AM UTC, comment #6:

forget it, state of MIT_SHM support is too poor atm,
there are missing pieces (the gdk_display for instance).

See patch #6546 for an alternative MIT-SHM support (gdk-specific)

Sandro Santilli <strk>
Project Member
Sat 12 Jul 2008 12:30:33 AM UTC, comment #5:

Go ahead and remove it - I'll fix the Win32 build afterwards.

Dossy Shiobara <dossy>
Sat 12 Jul 2008 12:01:33 AM UTC, comment #4:

Do you mind if I remove the hard-coded #undefine ENABLE_MIT_SHM in gui/gtk_glue_agg.h ? It's done at ./configure time (--enable-mit-shm) and stored in gnashconfig.h

Sandro Santilli <strk>
Project Member
Tue 24 Jun 2008 01:00:02 PM UTC, comment #3:

I see ENABLE_MIT_SHM undefined in gui/gtk_glue_agg.h, was
it a recent change ?
Anyway, I get frequent segfaults on resize.

Sandro Santilli <strk>
Project Member
Fri 20 Jun 2008 05:37:14 PM UTC, comment #2:

See also bug #23620 (closed) for other possible consequences of this bug.

Benjamin Wolsey <bwy>
Project Member
Wed 18 Jun 2008 10:56:30 AM UTC, comment #1:

NOTE: this might be related to the startstopped bug too: bug #23477

Sandro Santilli <strk>
Project Member
Wed 18 Jun 2008 06:51:43 AM UTC, original submission:

So there's another buffer overflow in the GTK/AGG. The sequence of events is as follows.

1. GtkGui::init() sets up the event handlers, window and drawing area and immediately shows them, without explicitly setting sizes.
2. A configure event is fired for the drawing area at, for example, 400x1 (a default).
3. The configure event is forwarded to the GtkAggGlue, which does nothing because the renderer has not been initialized with createRenderHandler() yet.
4. The GTK GUI calls createRenderHandler().
5. The GTK GUI calls setRenderHandlerSize(640, 480), for example.
6. The GTK GUI sets the drawing area widget size request to 640x480.
7. GTK GUI initialization finishes and rendering starts (but the configure event has not fired yet!)
8. GtkGui::render() is called with 640x480 invalidated bounds. By this time, the renderer has finished rendering into its own buffer.
9. GtkAggGlue copies the 640x480 buffer into the drawing area. (But at this time, the configure event still has not fired, so the allocated space for the widget is still 400x1! So this is a segfault, unless you are lucky.)
10. the configure event would have fired with 640x480, if we were still a running process.

Fixing this isn't quite so easy. And in fact, I think we might have to find a more permanent solution to issues like this because the GtkGui doesn't enforce the sequence of events.

I'm assigning this to bwy because he's done considerable work on the GTK GUI so he might have some ideas here.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by strk
  • -unavailable- added by dossy (Posted a comment)
  • -unavailable- added by strk
  • -unavailable- added by bwy (Posted a comment)
  • -unavailable- added by strk (Posted a comment)
  • -unavailable- added by bjacques (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 23 Jan 2009 09:33:45 AM UTCbwyStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Thu 13 Nov 2008 10:48:08 PM UTCbjacquesStatusIn Progress=>Ready For Test
    Sat 18 Oct 2008 09:12:46 PM UTCbjacquesStatusPostponed=>In Progress
    Sun 12 Oct 2008 01:42:44 PM UTCstrkStatusNone=>Postponed
      Assigned tobwy=>bjacques
      Summarysegfault resizing w/ GTK=>segfault/crashes resizing w/ GTK and MIT-SHM enabled
    Sat 12 Jul 2008 12:37:24 AM UTCstrkCarbon-Copy-=>Added udog
    Sat 12 Jul 2008 12:01:33 AM UTCstrkCarbon-Copy-=>Added dossy
    Wed 09 Jul 2008 11:07:09 PM UTCstrkSeverity3 - Normal=>6 - Security
    Wed 09 Jul 2008 11:02:31 PM UTCbjacquesSummarysegfault with MIT-SHM=>segfault resizing w/ GTK

    Back to the top


    Powered by Savane 3.1-cleanup1