Re: [Ekiga-devel-list] Crash in opal-gmconf-bridge.cpp

[Julien Puydt <jpuydt free fr>]

Matthias Schneider a écrit :
> Quoting Damien Sandras <dsandras seconix com>:
>
> > Le vendredi 11 avril 2008 à 11:26 +0200, Matthias Schneider a écrit :
> > > Quoting Julien Puydt <jpuydt free fr>:
> > >
> > > > Hi,
> > > >
> > > > again, there is a problem with the bridge : it gets something from
> > > > gmconf, doesn't check before use, and triggers a floating point
> > exception.
> > > > I'm not sure whether the issue is in ekiga or in opal though :
> > > > 0x08132c71 in GMManager::set_video_options (this=0x8303238,
> > > >      options= 0xbfba9fec) at /usr/include/opal/opal/mediafmt.h:877
> > > > 877	    ) { PWaitAndSignal m(_mutex); MakeUnique(); return m_info !=
> > > > NULL && m_info->SetOptionInteger(name, value); }
> > > > (gdb) bt
> > > > #0  0x08132c71 in GMManager::set_video_options (this=0x8303238,
> > > >      options= 0xbfba9fec) at /usr/include/opal/opal/mediafmt.h:877
> > > > #1  0x0814d3d8 in Opal::ConfBridge::on_property_changed (this=0x8319728,
> > > >      key= 0xbfbaa084, entry=0x8290e60) at
> > > > endpoints/opal-gmconf-bridge.cpp:127
> > > >
> > > > The lines 126 and 127 of the bridge read like :
> > > >      options.maximum_frame_rate = gm_conf_entry_get_int (entry);
> > > >      manager.set_video_options (options);
> > > > if for some reason zero is returned, then we have a crash.
> > > >
> > > > Snark
> > > Sorry Snark,
> > > this is my fault, I renamed that setting yesterday assuming its only used
> > by the
> > >  vidinput-gmconf-bridge, where correct checks preventing your issue are
> > being
> > > done. I can offer to fix it tonight, or you can simply copy and paste the
> > > section where its being read in the vidinput-gmconf-bridge to the opal
> > bridge.
> > I think the checks should be done in GMManager::set_video_options too.
> > It would prevent crashes. (that is what I do).
> >
> > Btw, I cleaned a lot of things ;-)
> > --
> >  _     Damien Sandras
>
> Hm, I think I mentioned that already, but in my opinion the checks should be
> done what is now the bridge. In my opinion there should not even be such a
> thing like a bridge and its functionality be part of a service. In that way,
> all methods that set stuff in the engine calle by what is now the bridge could
> be private.  However this is rather a post-3.00 idea.
>
> I have preferred to do all the checking in the bridge because this code usually
> get rather long and does not add anything helpful to understand the actual code
> in my cores. It will only make the core functions longer, something I would like
> to avoid.

The checks have to be done where it fits. If an object has a method 
which gives a crash when receiving some arguments, then it's not the 
calls to the method which have to be protected, but the method itself 
which much be changed!

You don't have to check what you get from gmconf if the method you call 
filters bad values correctly.

Snark