flac dev - Aug 2011 - Memory leak

During my current dealings with the FLAC library I think I discovered a memory
leak. After an encoder stream has finish()'ed, I believe you are supposed to
use it again by calling init(). However, when verification is enabled, the
init() routine will create a new stream decoder (to verify the data) without
deleting (or reusing) the existing one. A small program demonstrating this is
pasted here:

 $ cat main.cc 

#include <cstdlib>
#include <string>
#include <iostream>
#include <FLAC++/encoder.h>

class FlacWriter : public FLAC::Encoder::File
{
 public:
  inline bool dataStart(std::string const &filename);
  inline bool dataUpdate(char const *const data, int len);
  inline bool dataFinish();
};

inline bool FlacWriter::dataStart(std::string const &filename)
{
  if (set_verify(true) && init(filename) ==
FLAC__STREAM_ENCODER_INIT_STATUS_OK)
    return true;
  return false;
}

inline bool FlacWriter::dataUpdate(char const *const data, int len)
{
  return true;  // handle data here and call process(_interleaved)
}

inline bool FlacWriter::dataFinish()
{
  if (!finish())
    return false;
  return true;
}

int main(int argc, char *argv[])
{
  int numencodes = argc > 1 ? atoi(argv[1]) : 2;
  if (!numencodes)
    numencodes = 2;

  FlacWriter fw;

  for (int i = 0; i < numencodes; ++i)
  {
    if (!fw.dataStart("filname.flac"))
    {
      std::cout << "Failed init!" << std::endl;
      return 1;
    }

    while (true) // normally read audio data here in blocks, and update
FlacWriter
    {
      if (!fw.dataUpdate("audio data read from file", 26))
      {
        std::cout << "Failed update!" << std::endl;
        return 1;
      }
      break;
    }
        
    if (!fw.dataFinish())
    {
      std::cout << "Failed finish!" << std::endl;
      return 1;
    }
  }
  return 0;
}

As can be seen from valgrind output, when the encoder is not reused after
finish(), no leaks occur:

 $ g++ -Wall -O3 main.cc -lFLAC++ 
 $ valgrind --leak-check=full ./a.out 1
==30494== Memcheck, a memory error detector
==30494== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==30494== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==30494== Command: ./a.out 1
==30494== 
==30494== 
==30494== HEAP SUMMARY:
==30494==     in use at exit: 0 bytes in 0 blocks
==30494==   total heap usage: 28 allocs, 28 frees, 144,168 bytes allocated
==30494== 
==30494== All heap blocks were freed -- no leaks are possible
==30494== 
==30494== For counts of detected and suppressed errors, rerun with: -v
==30494== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 20 from 8)

However for each time init() is called again on the encoder, a constant amount
of bytes are not deleted:

 $ valgrind --leak-check=full ./a.out 2
==30495== Memcheck, a memory error detector
==30495== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==30495== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==30495== Command: ./a.out 2
==30495== 
==30495== 
==30495== HEAP SUMMARY:
==30495==     in use at exit: 6,772 bytes in 5 blocks
==30495==   total heap usage: 52 allocs, 47 frees, 275,712 bytes allocated
==30495== 
==30495== 6,772 (8 direct, 6,764 indirect) bytes in 1 blocks are definitely lost
in loss record 5 of 5
==30495==    at 0x400522F: calloc (vg_replace_malloc.c:418)
==30495==    by 0x4068847: FLAC__stream_decoder_new (stream_decoder.c:284)
==30495==    by 0x4074911: init_stream_internal_ (stream_encoder.c:1007)
==30495==    by 0x4075A98: init_FILE_internal_ (stream_encoder.c:1221)
==30495==    by 0x401EE65: FLAC::Encoder::File::init(char const*)
(stream_encoder.cpp:459)
==30495==    by 0x401E9FC: FLAC::Encoder::File::init(std::string const&)
(stream_encoder.cpp:464)
==30495==    by 0x804A300: main (in /home/svandijk/programming/FLACLEAK/a.out)
==30495== 
==30495== LEAK SUMMARY:
==30495==    definitely lost: 8 bytes in 1 blocks
==30495==    indirectly lost: 6,764 bytes in 4 blocks
==30495==      possibly lost: 0 bytes in 0 blocks
==30495==    still reachable: 0 bytes in 0 blocks
==30495==         suppressed: 0 bytes in 0 blocks
==30495== 
==30495== For counts of detected and suppressed errors, rerun with: -v
==30495== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 20 from 8)

In this example I used FLAC::Encoder::File, but the same thing happens with
FLAC::Encoder::Stream and, from reading the source, with the C API, as long as
one does not call FLAC__stream_encoder_delete() and FLAC__stream_encoder_new()
between finish()'ing and init()'ing a stream.

The fix will have to be in init_stream_internal_() in
src/libFLAC/stream_encoder.c:1007 (in libFLAC-1.2.1), where it should checked
whether encoder->private_->verify.decoder == NULL before calling
FLAC__stream_decoder_new(). If a decoder is already set (from a previous call to
init(), it should first be deleted or (preferably) reused. Alternatively, the
verification decoder can be deleted when finish() is called on the encoder that
contains it.

Workarounds are to not reuse finished streams (though explicitly allowed by the
API), disable verification, or always use the C API and delete() and new() the
encoder after calling finish().

Please correct me if wrong about any of these things.

Thanks,
Bas Timmer

After some more testing, and reading the source, the finish() is indeed called
on the verification decoder when its container encoder is finished. This
correctly leaves the decoder in the FLAC__STREAM_DECODER_UNINITIALIZED state,
ready to be reinitialized (or return an error).

So, all that needs to be done is check for NULL when the encoder is initialized
and not call FLAC__stream_decoder_new() when a decoder is already allocated.

I have submitted a patch on the flac tracker here:
https://sourceforge.net/tracker/?func=detail&aid=3390048&group_id=13478&atid=313478

thanks,
Bas Timmer