[Flac-dev] [PATCH] Memory issue and cast causing failures when adding metadata

[Josh Coalson]

I looked at this in more detail.  it's not clear from the current
c++ docs, but since the c++ call uses the underlying C call
FLAC__*_encoder_set_metadata(), the client is also bound by the
same rule in the C layer that metadata objects persist until the
init call; see:

http://flac.sourceforge.net/api/group__flac__stream__encoder.html#a23

you could argue that it is an unnecessary restriction in the c++
layer that can be worked around with your patch but I wanted it to
be consistent with the C layer and also the other overloaded
version
FLAC::Encoder::File::set_metadata(::FLAC__StreamMetadata **metadata,
unsigned num_blocks)

the newer (unreleased) c++ doxygen docs refer to the C call
documentation.

but there *is* a bug in the cast, the C cast should be const'ed to
trigger the correct conversion method in FLAC::Metadata::Prototype
     inline operator const ::FLAC__StreamMetadata *() const;

I'll fix that.

Josh

--- "King, Joshua" <Joshua.King at dsto.defence.gov.au> wrote:

> Hi,
> 
> We are using the libFLAC++ library to encode FLAC files with Vorbis
> comments in them and have had the FLAC 1.1.2 version fail
> consistently
> (access violation or segmentation fault) on both Windows and Linux.
> We
> tracked this down to the metadata code in the C++ API.
> 
> The problem is in src/libFLAC++/file_encoder.cpp.
> 
> There are two problems - firstly, a C-style cast is hiding a bad
> conversion from the pointer to a C++ object to the C API's struct for
> metadata and secondly, the method uses an array internal to the
> method
> to set the metadata using the C API.
> 
> I have attached some test code which causes the problem.
> 
> The underlying problem is that the set_metadata calls don't keep a
> copy
> of the metadata information, even though it is not read (and written
> to
> the stream) until the init call. However the objects inside the array
> must also remain valid (this is the library user's responsibility) so
> I
> also added a note in the header documentation.
> 
> I have attached a patch to fix the cast problem, and also to make the
> C++ API hold onto its array of metadata blocks (but not copied). This
> avoids changing the C library behaviour, though I would think it
> better
> to have the metadata blocks copied upon the set_metadata call (ie, in
> src/libFLAC/stream_encoder.c) instead.
> 
> Also note that I have only changed the file_decoder, a corresponding
> problem seems to exist in the code for
> seekable_stream_encoder/stream_encoder as well.
> 
> Thanks,
> 
> 
> Joshua King
> Defence Science and Technology Organisation
> Tel: (08) 9553 4305


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com