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

 
 

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

Submitter:  Bastiaan Jacques <bjacques>
Submitted:  Fri 18 Feb 2011 09:24:54 PM UTC
   
 
Category:  sound Severity:  4 - Important
Release:  master Status:  Fixed
Privacy:  Public Assigned to:  strk
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group Member
Wed 23 Feb 2011 08:30:20 AM UTC, comment #6: 

Pushed 414f77a41775378903e24603aa97f0228036fa18

Sandro Santilli <strk>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group 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>
Group Member

 

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

Attach Files:
   
   
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
  • -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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-03-02 bjacques StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2011-02-23 strk StatusIn Progress Ready For Test
    2011-02-23 strk Assigned tobjacques strk
    2011-02-22 strk Attached File- Added Sound.Longer.Frame.Lifetime.patch, #22762
    2011-02-18 bjacques Attached File- Added gnash-thread-fix-bad-read.diff, #22727
        Categoryvideo sound

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code