bugGnash - The GNU Flash player - Bugs: bug #32540, Invalid read of size 4 in...

 
 

bug #32540: Invalid read of size 4 in MediaParser::nextAudioFrameTimestamp()

Submitted by:  Bastiaan Jacques <bjacques>
Submitted on:  Fri 18 Feb 2011 09:24:54 PM UTC  
 
Category: soundSeverity: 4 - Important
Release: masterStatus: Fixed
Privacy: PublicAssigned to: Sandro Santilli <strk>
Open/Closed: Closed

Add a New Comment (Rich MarkupRich 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)

Wed 23 Feb 2011 08:39:38 AM UTC, comment #7:

See bug #32574 and bug #32575 for other issues related to this movie

Sandro Santilli <strk>
Project MemberIn charge of this item.
Wed 23 Feb 2011 08:30:20 AM UTC, comment #6:

Pushed 414f77a41775378903e24603aa97f0228036fa18

Sandro Santilli <strk>
Project MemberIn charge of this item.
Wed 23 Feb 2011 08:26:42 AM UTC, comment #5:

I'm working on a simple improvement to your patch.
Seems to be the less destruptive way to go, after I've found
the peek functions are really unused outside of the base class.

So my patch makes the peek* functions private, document the locking mechanism (the requirement for callers of peek* to be holding a lock) and making this symmetric for audio and video.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Tue 22 Feb 2011 10:04:29 PM UTC, comment #4:

@bjacques: could you try the attached patch ? It makes lifetime of the fetched frame slightly longer

(file #22762)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Tue 22 Feb 2011 09:54:25 PM UTC, comment #3:

For the record, the issue was that while we do properly lock the container of A/V frames, we have no mechanism to ensure that the single frame fetched from the container is kept alive while used by a thread.

In particular, this specific case has a thread peeking first
frame and another pulling it off (obtaining ownership).
By the time the first thread attempts to dereference the peeked
frame, the other thread deleted it already.

Bastiaan patch would keep the queue locked for the whole (short) time of both peeking and querying the frame for timestamp.
Being a short time it'd be fine by me. Only concern is that it
drops locking from another function, which would require going trough all callers to be sure about what the consequences could be.
A safer approach might be to leave the existing lockings in place and just add longer locking in nextFrameTimestamp avoiding the call to peekNextFrame (or extend the peekNextFrame interface to allow specifying whether or not one wants locking, or a new function or something along those lines).
Also, for symmetricity, the same behaviour should be made for Audio and Video.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Tue 22 Feb 2011 06:04:47 PM UTC, comment #2:

A frame fetched trough nextAudioFrame should no more be in the MediaParser buffer. Do you get a message, with verbosity>1, about frames being reordered ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 18 Feb 2011 10:38:29 PM UTC, comment #1:

Sandro, can you have a look at this patch?

(file #22727)

Bastiaan Jacques <bjacques>
Project Member
Fri 18 Feb 2011 09:24:54 PM UTC, original submission:

STR: valgrind gui/.libs/lt-gtk-gnash -M gst http://download.omroep.nl/vpro/luisterpaal/new-luisterpaal/player.swf?list=/feeds/luisterpaalPlaylist?luisterpaalnr=44452648 -U http://3voor12.vpro.nl/speler/luisterpaal/44452648#luisterpaal.44452648

Press the 'next item' button.

==11849== Thread 1:
==11849== Invalid read of size 4
==11849== at 0x4862574: gnash::media::MediaParser::nextAudioFrameTimestamp(unsigned long long&) const (MediaParser.cpp:226)
==11849== by 0x436D247: gnash::Sound_as::getPosition() const (Sound_as.cpp:752)
==11849== by 0x436BE1F: gnash::Sound_as::update() (Sound_as.cpp:341)
==11849== by 0x4277DC1: gnash::(anonymous namespace)::ExecuteCallback::operator()(gnash::as_object const*) const (movie_root.cpp:108)
==11849== by 0x428113D: gnash::(anonymous namespace)::ExecuteCallback std::for_each<__gnu_cxx::__normal_iterator<gnash::as_object*, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, gnash::(anonymous namespace)::ExecuteCallback>(__gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, __gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object*> > >, gnash::(anonymous namespace)::ExecuteCallback) (stl_algo.h:4275)
==11849== by 0x427C6E1: gnash::movie_root::executeAdvanceCallbacks() (movie_root.cpp:1474)
==11849== by 0x427A94E: gnash::movie_root::advance() (movie_root.cpp:855)
==11849== by 0x80BB1E1: gnash::Gui::advanceMovie(bool) (gui.cpp:970)
==11849== by 0x80E7184: gnash::Gui::advance_movie(gnash::Gui*) (gui.h:285)
==11849== by 0x3D98FC: g_timeout_dispatch (gmain.c:3585)
==11849== by 0x3D8191: g_main_context_dispatch (gmain.c:2149)
==11849== by 0x3D8977: g_main_context_iterate.clone.5 (gmain.c:2780)
==11849== Address 0x78982d4 is 12 bytes inside a block of size 20 free'd
==11849== at 0x4005821: operator delete(void*) (vg_replace_malloc.c:346)
==11849== by 0x437029F: std::auto_ptr<gnash::media::EncodedAudioFrame>::~auto_ptr() (in /home/bastiaan/gnash/obj-master/libcore/.libs/libgnashcore-0.8.9dev.so)
==11849== by 0x436D44E: gnash::Sound_as::getAudio(short*, unsigned int, bool&) (Sound_as.cpp:773)
==11849== by 0x436D60B: gnash::Sound_as::getAudioWrapper(void, short, unsigned int, bool&) (Sound_as.cpp:846)
==11849== by 0x46AF0BE: gnash::sound::AuxStream::fetchSamples(short*, unsigned int) (AuxStream.h:47)
==11849== by 0x46ADD8D: gnash::sound::sound_handler::fetchSamples(short*, unsigned int) (sound_handler.cpp:583)
==11849== by 0x46C64B4: gnash::sound::SDL_sound_handler::fetchSamples(short*, unsigned int) (sound_handler_sdl.cpp:231)
==11849== by 0x46C65CF: gnash::sound::SDL_sound_handler::sdl_audio_callback(void, unsigned char, int) (sound_handler_sdl.cpp:270)
==11849== by 0x2817FC4: SDL_RunAudio (SDL_audio.c:198)
==11849== by 0x2820B3E: SDL_RunThread (SDL_thread.c:204)
==11849== by 0x286C2BD: RunThread (SDL_systhread.c:47)
==11849== by 0x313E98: start_thread (pthread_create.c:301)
==11849==
==11849== Invalid read of size 4
==11849== at 0x4862577: gnash::media::MediaParser::nextAudioFrameTimestamp(unsigned long long&) const (MediaParser.cpp:226)
==11849== by 0x436D247: gnash::Sound_as::getPosition() const (Sound_as.cpp:752)
==11849== by 0x436BE1F: gnash::Sound_as::update() (Sound_as.cpp:341)
==11849== by 0x4277DC1: gnash::(anonymous namespace)::ExecuteCallback::operator()(gnash::as_object const*) const (movie_root.cpp:108)
==11849== by 0x428113D: gnash::(anonymous namespace)::ExecuteCallback std::for_each<__gnu_cxx::__normal_iterator<gnash::as_object*, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, gnash::(anonymous namespace)::ExecuteCallback>(__gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, __gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object*> > >, gnash::(anonymous namespace)::ExecuteCallback) (stl_algo.h:4275)
==11849== by 0x427C6E1: gnash::movie_root::executeAdvanceCallbacks() (movie_root.cpp:1474)
==11849== by 0x427A94E: gnash::movie_root::advance() (movie_root.cpp:855)
==11849== by 0x80BB1E1: gnash::Gui::advanceMovie(bool) (gui.cpp:970)
==11849== by 0x80E7184: gnash::Gui::advance_movie(gnash::Gui*) (gui.h:285)
==11849== by 0x3D98FC: g_timeout_dispatch (gmain.c:3585)
==11849== by 0x3D8191: g_main_context_dispatch (gmain.c:2149)
==11849== by 0x3D8977: g_main_context_iterate.clone.5 (gmain.c:2780)
==11849== Address 0x78982d0 is 8 bytes inside a block of size 20 free'd
==11849== at 0x4005821: operator delete(void*) (vg_replace_malloc.c:346)
==11849== by 0x437029F: std::auto_ptr<gnash::media::EncodedAudioFrame>::~auto_ptr() (in /home/bastiaan/gnash/obj-master/libcore/.libs/libgnashcore-0.8.9dev.so)
==11849== by 0x436D44E: gnash::Sound_as::getAudio(short*, unsigned int, bool&) (Sound_as.cpp:773)
==11849== by 0x436D60B: gnash::Sound_as::getAudioWrapper(void, short, unsigned int, bool&) (Sound_as.cpp:846)
==11849== by 0x46AF0BE: gnash::sound::AuxStream::fetchSamples(short*, unsigned int) (AuxStream.h:47)
==11849== by 0x46ADD8D: gnash::sound::sound_handler::fetchSamples(short*, unsigned int) (sound_handler.cpp:583)
==11849== by 0x46C64B4: gnash::sound::SDL_sound_handler::fetchSamples(short*, unsigned int) (sound_handler_sdl.cpp:231)
==11849== by 0x46C65CF: gnash::sound::SDL_sound_handler::sdl_audio_callback(void, unsigned char, int) (sound_handler_sdl.cpp:270)
==11849== by 0x2817FC4: SDL_RunAudio (SDL_audio.c:198)
==11849== by 0x2820B3E: SDL_RunThread (SDL_thread.c:204)
==11849== by 0x286C2BD: RunThread (SDL_systhread.c:47)
==11849== by 0x313E98: start_thread (pthread_create.c:301)
==11849==

Bastiaan Jacques <bjacques>
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 #22762:  Sound.Longer.Frame.Lifetime.patch added by strk (815B - application/octet-stream)
file #22727:  gnash-thread-fix-bad-read.diff added by bjacques (932B - text/plain - fix)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 02 Mar 2011 08:42:58 PM UTCbjacquesStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 23 Feb 2011 08:30:20 AM UTCstrkStatusIn Progress=>Ready For Test
    Wed 23 Feb 2011 08:26:42 AM UTCstrkAssigned tobjacques=>strk
    Tue 22 Feb 2011 10:04:29 PM UTCstrkAttached File-=>Added Sound.Longer.Frame.Lifetime.patch, #22762
    Fri 18 Feb 2011 10:38:29 PM UTCbjacquesAttached File-=>Added gnash-thread-fix-bad-read.diff, #22727
      Categoryvideo=>sound

    Back to the top


    Powered by Savane 3.1-cleanup