bugGnash - The GNU Flash player - Bugs: bug #24240, memory errors in libamf...

 
 

bug #24240: memory errors in libamf (SharedObject)

Submitted by:  Sandro Santilli <strk>
Submitted on:  Thu 11 Sep 2008 12:48:51 AM UTC  
 
Category: RTMP and AMFSeverity: 6 - Security
Release: trunkStatus: Fixed
Privacy: PublicAssigned to: Rob Savoye <rsavoye>
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)

Mon 23 Feb 2009 09:04:31 PM UTC, comment #7:

I think this has been fixed for ages, just checking before closing this.

Rob Savoye <rsavoye>
Project AdministratorIn charge of this item.
Mon 06 Oct 2008 02:23:19 PM UTC, comment #6:

If by default libamf isn't being used, then there isn't a problem. I've always seen amny more problems when using valgrind with gprocessor, as Gnash itselg isn't valgrind clean. All the test cases that run in isolation are 100% valgrind clean on both 32 and 64 bit architectures.

In the current rtmp branch, all this code changed heavily anyway to use boost shared and scoped pointers, and this problem is gone. Since this required extensive changes to libamf, and this is a very minor problem, the proper fix will be migrated to trunk after the 0.8.4 release.

Rob Savoye <rsavoye>
Project AdministratorIn charge of this item.
Mon 06 Oct 2008 07:07:06 AM UTC, comment #5:

Can't confirm the fix. If you comment out the BUFFERED_AMF_SOL in libcore/asobj/SharedObject.cpp (thus enabling use of libamf)
and run SharedObjectTestRunner passing 'valgrind gprocessor' as first argument (to set player to use), this is what you get:

==18858== Conditional jump or move depends on uninitialised value(s)
==18858== at 0x48C513C: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:887)
==18858== by 0x48C4E99: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:780)
==18858== Conditional jump or move depends on uninitialised value(s)
==18858== at 0x48C4D01: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:707)
==18858== by 0x48C5154: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:898)
==18858== Use of uninitialised value of size 4
==18858== at 0x48C4D5B: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:707)
==18858== by 0x48C5154: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:898)
==18858== Invalid read of size 8
==18858== at 0x48C4FDE: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:710)
==18858== Address 0x5c35723 is 267 bytes inside a block of size 270 alloc'd
==18858== at 0x402309E: operator new[](unsigned) (vg_replace_malloc.c:268)
==18858== by 0x48DA94D: amf::SOL::readFile(std::string const&) (sol.cpp:340)

Sandro Santilli <strk>
Project Member
Sun 05 Oct 2008 06:17:06 PM UTC, comment #4:

Fixed, according to rsavoye.

Bastiaan Jacques <bjacques>
Project Member
Sun 14 Sep 2008 11:18:23 AM UTC, comment #3:

The memory errors are caused by reading past the end of the buffer. In extractProperty(), for some reason the first element in the buffer is skipped. If that is the only element (or if the element is larger than the "sane" size), that means we skip until the end of the buffer and then start reading:
And we subsequently let a condition depend on the value of that uninitialized memory:
which causes Valgrind to complain.

The attached patch demonstrates that not skipping the first element will prevent us from reading past the end of the buffer. However, this does not cause the tests to succeed.

I don't understand what the code was intended so I can't come up with a better patch.

That said, this code appears to be disabled currently...

(file #16481)

Bastiaan Jacques <bjacques>
Project Member
Thu 11 Sep 2008 12:49:20 PM UTC, comment #2:

FYI: defining BUFFERED_AMF_SOL in libcore/asobj/SharedObject.cpp
fixes the bug (and gives many XPASS).

Sandro Santilli <strk>
Project Member
Thu 11 Sep 2008 12:51:10 AM UTC, comment #1:

To modify SharedObjectTestRunner change line:

${PLAYER} ${SWFTEST}

to

libtool --mode=execute valgrind ${PLAYER} ${SWFTEST} 2> VL

Then run and check the VL file

Sandro Santilli <strk>
Project Member
Thu 11 Sep 2008 12:48:51 AM UTC, original submission:

Running misc-ming.all/SharedObjectTestRunner, appropriately
modified to engage valgrind, gives these memory errors:

==1146== Conditional jump or move depends on uninitialised value(s)
==1146== at 0x47AD5F6: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:887)
==1146== by 0x47C2DC6: amf::SOL::readFile(std::string const&) (sol.cpp:401)
==1146== by 0x4230587: gnash::SharedObject::readSOL(std::string const&) (SharedObject.cpp:369)
==1146== by 0x4234154: gnash::SharedObjectLibrary::getLocal(std::string const&, std::string const&) (SharedObject.cpp:356)
==1146== by 0x4234965: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:553)
==1146== by 0x40DC86A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==1146== by 0x4349004: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, std::auto_ptr<std::vector<gnash::as_value, std::allocator<gnash::as_value> > >, gnash::as_object*) (action.cpp:68)
==1146== by 0x4303464: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3450)
==1146== by 0x42F2611: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:159)
==1146== by 0x42F9865: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:439)
==1146== by 0x43404DC: gnash::ActionExec::operator()() (ActionExec.cpp:267)
==1146== by 0x413AAFD: gnash::GlobalCode::execute() (ExecutableCode.h:74)
==1146==
==1146== Conditional jump or move depends on uninitialised value(s)
==1146== at 0x47AD821: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:707)
==1146== by 0x47AD611: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:898)
==1146== by 0x47C2DC6: amf::SOL::readFile(std::string const&) (sol.cpp:401)
==1146== by 0x4230587: gnash::SharedObject::readSOL(std::string const&) (SharedObject.cpp:369)
==1146== by 0x4234154: gnash::SharedObjectLibrary::getLocal(std::string const&, std::string const&) (SharedObject.cpp:356)
==1146== by 0x4234965: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:553)
==1146== by 0x40DC86A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==1146== by 0x4349004: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, std::auto_ptr<std::vector<gnash::as_value, std::allocator<gnash::as_value> > >, gnash::as_object*) (action.cpp:68)
==1146== by 0x4303464: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3450)
==1146== by 0x42F2611: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:159)
==1146== by 0x42F9865: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:439)
==1146== by 0x43404DC: gnash::ActionExec::operator()() (ActionExec.cpp:267)
==1146==
==1146== Use of uninitialised value of size 4
==1146== at 0x47AD889: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:707)
==1146== by 0x47AD611: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:898)
==1146== by 0x47C2DC6: amf::SOL::readFile(std::string const&) (sol.cpp:401)
==1146== by 0x4230587: gnash::SharedObject::readSOL(std::string const&) (SharedObject.cpp:369)
==1146== by 0x4234154: gnash::SharedObjectLibrary::getLocal(std::string const&, std::string const&) (SharedObject.cpp:356)
==1146== by 0x4234965: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:553)
==1146== by 0x40DC86A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==1146== by 0x4349004: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, std::auto_ptr<std::vector<gnash::as_value, std::allocator<gnash::as_value> > >, gnash::as_object*) (action.cpp:68)
==1146== by 0x4303464: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3450)
==1146== by 0x42F2611: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:159)
==1146== by 0x42F9865: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:439)
==1146== by 0x43404DC: gnash::ActionExec::operator()() (ActionExec.cpp:267)
==1146==
==1146== Invalid read of size 8
==1146== at 0x47ADAC2: amf::AMF::extractAMF(unsigned char, unsigned char) (amf.cpp:710)
==1146== by 0x47AD611: amf::AMF::extractProperty(unsigned char, unsigned char) (amf.cpp:898)
==1146== by 0x47C2DC6: amf::SOL::readFile(std::string const&) (sol.cpp:401)
==1146== by 0x4230587: gnash::SharedObject::readSOL(std::string const&) (SharedObject.cpp:369)
==1146== by 0x4234154: gnash::SharedObjectLibrary::getLocal(std::string const&, std::string const&) (SharedObject.cpp:356)
==1146== by 0x4234965: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:553)
==1146== by 0x40DC86A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==1146== by 0x4349004: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, std::auto_ptr<std::vector<gnash::as_value, std::allocator<gnash::as_value> > >, gnash::as_object*) (action.cpp:68)
==1146== by 0x4303464: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3450)
==1146== by 0x42F2611: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:159)
==1146== by 0x42F9865: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:439)
==1146== by 0x43404DC: gnash::ActionExec::operator()() (ActionExec.cpp:267)
==1146== Address 0x5847382 is 186 bytes inside a block of size 189 alloc'd
==1146== at 0x4022F14: operator new[](unsigned) (vg_replace_malloc.c:268)
==1146== by 0x47C2AAE: amf::SOL::readFile(std::string const&) (sol.cpp:340)
==1146== by 0x4230587: gnash::SharedObject::readSOL(std::string const&) (SharedObject.cpp:369)
==1146== by 0x4234154: gnash::SharedObjectLibrary::getLocal(std::string const&, std::string const&) (SharedObject.cpp:356)
==1146== by 0x4234965: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:553)
==1146== by 0x40DC86A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==1146== by 0x4349004: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, std::auto_ptr<std::vector<gnash::as_value, std::allocator<gnash::as_value> > >, gnash::as_object*) (action.cpp:68)
==1146== by 0x4303464: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3450)
==1146== by 0x42F2611: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:159)

Sandro Santilli <strk>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #16481:  sharedobj.diff added by bjacques (1001B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by rsavoye (Posted a comment)
  • -unavailable- added by bjacques (Updated the item)
  • -unavailable- added by strk (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 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 23 Nov 2009 10:32:02 PM UTCstrkStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Mon 23 Feb 2009 09:04:31 PM UTCrsavoyeStatusPostponed=>Ready For Test
    Mon 06 Oct 2008 02:23:19 PM UTCrsavoyeStatusConfirmed=>Postponed
    Mon 06 Oct 2008 07:07:06 AM UTCstrkStatusReady For Test=>Confirmed
    Sun 05 Oct 2008 06:17:05 PM UTCbjacquesStatusConfirmed=>Ready For Test
    Sun 14 Sep 2008 11:24:20 AM UTCbjacquesReleaseNone=>trunk
      StatusNone=>Confirmed
      Assigned toNone=>rsavoye
    Sun 14 Sep 2008 11:18:23 AM UTCbjacquesAttached File-=>Added sharedobj.diff, #16481

    Back to the top


    Powered by Savane 3.1-cleanup1