flac dev - Aug 2006 - [PATCH] Memory issue and cast causing failures when adding metadata

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

IMPORTANT: This email remains the property of the Australian Defence
Organisation and is subject to the jurisdiction of section 70 of the
Crimes Act 1914. If you have received this email in error, you are
requested to contact the sender and delete the email.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flac-set-metadata-fix.patch
Type: application/octet-stream
Size: 3968 bytes
Desc: flac-set-metadata-fix.patch
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20060822/7a30e3ec/flac-set-metadata-fix.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.cpp
Type: application/octet-stream
Size: 2307 bytes
Desc: test.cpp
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20060822/7a30e3ec/test.obj

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@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