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

 
 

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

Submitter:  Bastiaan Jacques <bjacques>
Submitted:  Wed 18 Jun 2008 06:51:43 AM UTC
   
 
Category:  gui-gtk Severity:  6 - Security
Release:  None Status:  Fixed
Privacy:  Public Assigned to:  bjacques
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group Member
Fri 10 Oct 2008 12:43:10 PM UTC, comment #12: 

Status of this bug?

Bastiaan Jacques <bjacques>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-01-23 bwy StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2008-11-13 bjacques StatusIn Progress Ready For Test
    2008-10-18 bjacques StatusPostponed In Progress
    2008-10-12 strk StatusNone Postponed
        Assigned tobwy bjacques
        Summarysegfault resizing w/ GTK segfault/crashes resizing w/ GTK and MIT-SHM enabled
    2008-07-12 strk Carbon-Copy- Added udog
    2008-07-12 strk Carbon-Copy- Added dossy
    2008-07-09 strk Severity3 - Normal 6 - Security
    2008-07-09 bjacques Summarysegfault with MIT-SHM segfault resizing w/ GTK

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code