Skip to main content ↑ ↓

Notice

Please note that most of the software linked on this forum is likely to be safe to use. If you are unsure, feel free to ask in the relevant topics, or send a private message to an administrator or moderator. To help curb the problems of false positives, or in the event that you do find actual malware, you can contribute through the article linked here.
Topic: Memory leaks due to Metadata object vorbis comment API ? (Read 2700 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Memory leaks due to Metadata object vorbis comment API ?

Hi List,

I recently was assigned a task to port FLAC Encoder to our embedded platform. Thanks to OO-like design of the libFLAC and thorough documentation, that porting went like a charm. I had some problems with chmod/chown like routines while porting but I was able to safely remove that piece of code without any trouble.

I have observed that the my application FLAC Encoder failes in restartability test after about 1500 restarts. Narrowing down the problem shows that I am having problem with multiple calls of FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair() and/or FLAC__metadata_object_vorbiscomment_append_comment() routines, i.e. If I have vorbis comment in my metadata with copy flag == true, my application's restartability is affected. To test my observation, I took the simple encoder example available with the FLAC package and added a couple of vorbis comments in it as follows,


    #if HAVE_CONFIG_H
    #  include <config.h>
    #endif

    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include "FLAC_metadata.h"
    #include "FLAC_stream_encoder.h"

    static char outFileName[256] = {"encAudio.flac"};
    static unsigned char FLAC_tagTitle[] = {"Title"};
    static const unsigned char FLAC_tagArtist[] = {"Artist"};
    static const unsigned char FLAC_tagAlbum[] = {"Album"};
    static const unsigned char FLAC_tagYear[] = {"2008"};
    static const unsigned char FLAC_tagGenre[] = {"Audio Track"};
    static const unsigned char defFileName[16] = {"encAUDIO.flac"};

    static void progress_callback(const FLAC__StreamEncoder *encoder, FLAC__uint64 bytes_written, FLAC__uint64 samples_written, unsigned frames_written, unsigned total_frames_estimate, void *client_data);

    #define READSIZE 1024

    static unsigned total_samples = 0; /* can use a 32-bit number due to WAVE size limitations */
    static FLAC__byte buffer[READSIZE/*samples*/ * 2/*bytes_per_sample*/ * 2/*channels*/]; /* we read the WAVE data into here */
    static FLAC__int32 pcm[READSIZE/*samples*/ * 2/*channels*/];

    int main(int argc, char *argv[])
    {
        FLAC__bool ok = true;
        FLAC__StreamEncoder *encoder = 0;
        FLAC__StreamEncoderInitStatus init_status;
        FLAC__StreamMetadata *metadata[2];
        FLAC__StreamMetadata_VorbisComment_Entry entry;
        FILE *fin;
        unsigned sample_rate = 0;
        unsigned channels = 0;
        unsigned bps = 0;

        if(argc != 3) {
            fprintf(stderr, "usage: %s infile.wav outfile.flac\n", argv[0]);
            return 1;
        }

        if((fin = fopen(argv[1], "rb")) == NULL) {
            fprintf(stderr, "ERROR: opening %s for output\n", argv[1]);
            return 1;
        }

        /* read wav header and validate it */
        if(
            fread(buffer, 1, 44, fin) != 44 ||
            memcmp(buffer, "RIFF", 4) ||
            memcmp(buffer+8, "WAVEfmt \020\000\000\000\001\000\002\000", 16) ||
            memcmp(buffer+32, "\004\000\020\000data", 8)
        ) {
            fprintf(stderr, "ERROR: invalid/unsupported WAVE file, only 16bps stereo WAVE in canonical form allowed\n");
            fclose(fin);
            return 1;
        }
        sample_rate = ((((((unsigned)buffer[27] << 8) | buffer[26]) << 8) | buffer[25]) << 8) | buffer[24];
        printf("sampleRate:%d\n", sample_rate);
        channels = 2;
        bps = 16;
        total_samples = (((((((unsigned)buffer[43] << 8) | buffer[42]) << 8) | buffer[41]) << 8) | buffer[40]) / 4;
     
        /* allocate the encoder */
        if((encoder = FLAC__stream_encoder_new()) == NULL) {
            fprintf(stderr, "ERROR: allocating encoder\n");
            fclose(fin);
            return 1;
        }

        ok &= FLAC__stream_encoder_set_verify(encoder, true);
        ok &= FLAC__stream_encoder_set_compression_level(encoder, 5);
        ok &= FLAC__stream_encoder_set_channels(encoder, channels);
        ok &= FLAC__stream_encoder_set_bits_per_sample(encoder, bps);
        ok &= FLAC__stream_encoder_set_sample_rate(encoder, sample_rate);
        ok &= FLAC__stream_encoder_set_total_samples_estimate(encoder, total_samples);

        if(  (metadata[0] = FLAC__metadata_object_new(FLAC__METADATA_TYPE_PADDING)) == NULL ||
            (metadata[1] = FLAC__metadata_object_new(FLAC__METADATA_TYPE_VORBIS_COMMENT)) == NULL)
            ok = false;

        metadata[0]->length = 512; /* set the padding length */
    #if BUGGY
        ok &= FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair(&entry, "TITLE", FLAC_tagTitle);
        ok &= FLAC__metadata_object_vorbiscomment_append_comment(metadata[1], entry, /*copy=*/true);
        ok &= FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair(&entry, "ARTIST", FLAC_tagArtist);
        ok &= FLAC__metadata_object_vorbiscomment_append_comment(metadata[1], entry, /*copy=*/true);
        ok &= FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair(&entry, "ALBUM", FLAC_tagAlbum);
        ok &= FLAC__metadata_object_vorbiscomment_append_comment(metadata[1], entry, /*copy=*/true);
        ok &= FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair(&entry, "GENRE", FLAC_tagGenre);
        ok &= FLAC__metadata_object_vorbiscomment_append_comment(metadata[1], entry, /*copy=*/true);
        ok &= FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair(&entry, "DATE", FLAC_tagYear);
        ok &= FLAC__metadata_object_vorbiscomment_append_comment(metadata[1], entry, /*copy=*/true);
    #endif 
        if (!FLAC__stream_encoder_set_metadata(encoder, metadata, 2))
            return FLAC__STREAM_ENCODER_CLIENT_ERROR;

        /* initialize encoder */
        if(ok) {
            init_status = FLAC__stream_encoder_init_file(encoder, argv[2], progress_callback, /*client_data=*/NULL);
            if(init_status != FLAC__STREAM_ENCODER_INIT_STATUS_OK) {
                fprintf(stderr, "ERROR: initializing encoder: %s\n", FLAC__StreamEncoderInitStatusString[init_status]);
                ok = false;
            }
        }

        /* read blocks of samples from WAVE file and feed to encoder */
        if(ok) {
            size_t left = (size_t)total_samples;
            while(ok && left) {
                size_t need = (left>READSIZE? (size_t)READSIZE : (size_t)left);
                if(fread(buffer, channels*(bps/8), need, fin) != need) {
                    fprintf(stderr, "ERROR: reading from WAVE file\n");
                    ok = false;
                }
                else {
                    /* convert the packed little-endian 16-bit PCM samples from WAVE into an interleaved FLAC__int32 buffer for libFLAC */
                    size_t i;
                    for(i = 0; i < need*channels; i++) {
                        /* inefficient but simple and works on big- or little-endian machines */
                        pcm = (FLAC__int32)(((FLAC__int16)(FLAC__int8)buffer[2*i+1] << 8) | (FLAC__int16)buffer[2*i]);
                    }
                    /* feed samples to encoder */
                    ok = FLAC__stream_encoder_process_interleaved(encoder, pcm, need);
                }
                left -= need;
            }
        }

        ok &= FLAC__stream_encoder_finish(encoder);

        fprintf(stderr, "encoding: %s\n", ok? "succeeded" : "FAILED");
        fprintf(stderr, "  state: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);

        /* now that encoding is finished, the metadata can be freed */
        FLAC__metadata_object_delete(metadata[0]);
        FLAC__metadata_object_delete(metadata[1]);
        FLAC__stream_encoder_delete(encoder);
     
        fclose(fin);

        return 0;
    }

    void progress_callback(const FLAC__StreamEncoder *encoder, FLAC__uint64 bytes_written, FLAC__uint64 samples_written, unsigned frames_written, unsigned total_frames_estimate, void *client_data)
    {
        (void)encoder, (void)client_data;

    #ifdef _MSC_VER
        fprintf(stderr, "wrote %I64u bytes, %I64u/%u samples, %u/%u frames\n", bytes_written, samples_written, total_samples, frames_written, total_frames_estimate);
    #else
        fprintf(stderr, "wrote %llu bytes, %llu/%u samples, %u/%u frames\n", bytes_written, samples_written, total_samples, frames_written, total_frames_estimate);
    #endif
    }


Then I compiled it with -g option and fed it to valgrind using tool=memcheck. and here is the output of Valgrind,

    ... wrote 634814 bytes, 292570/292570 samples, 72/72 frames
    encoding: succeeded
      state: FLAC__STREAM_ENCODER_UNINITIALIZED
    ==21449==
    ==21449== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 13 from 1)
    ==21449== malloc/free: in use at exit: 97 bytes in 5 blocks.
    ==21449== malloc/free: 78 allocs, 73 frees, 477,278 bytes allocated.
    ==21449== For counts of detected errors, rerun with: -v
    ==21449== searching for pointers to 5 not-freed blocks.
    ==21449== checked 67,296 bytes.
    ==21449==
    ==21449== 97 bytes in 5 blocks are definitely lost in loss record 1 of 1
    ==21449==    at 0x401A826: malloc (vg_replace_malloc.c:149)
    ==21449==    by 0x804C66F: FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair (in /home/XXX/workspace/FLACEnc/FLACEncoder)
    ==21449==    by 0x80488AC: (within /home/XXX/workspace/FLACEnc/FLACEncoder)
    ==21449==
    ==21449== LEAK SUMMARY:
    ==21449==    definitely lost: 97 bytes in 5 blocks.
    ==21449==      possibly lost: 0 bytes in 0 blocks.
    ==21449==    still reachable: 0 bytes in 0 blocks.
    ==21449==        suppressed: 0 bytes in 0 blocks.


Above results are produced with libFLAC(vanilla) available with FLAC v1.2.1 on a linux distro.

I have observed that If I use VORBIS_COMMENT metadata with copy == true, I get memory leaks, but with copy==false, I DONT get memory leaks.

I guess by asking the question, I have solved my problem of leak but the question itself stands still.

I dont know if my question is right or not,
"is this behavior desired, should we get a memory leak if copy == true, when should we set copy and when should we unset it"?

Just  before pressing Send button, I also tested If this behavior is useful for String pointers to the FLAC__metadata_object_vorbiscomment_append_comment() routine but It didnt help the leak!!!

Thanks all for your time.

Regards,
Nabsha

Memory leaks due to Metadata object vorbis comment API ?

Reply #1 –
when you do
Code: [Select]
ok &= FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair(&entry, "TITLE", FLAC_tagTitle);
this uses malloc to make a new string "name=value" and load this pointer into the entry struct.  then when you do

Code: [Select]
ok &= FLAC__metadata_object_vorbiscomment_append_comment(metadata[1], entry, /*copy=*/true);
(i.e. with copy=true), it loads the metadata with a copy of your entry.  after this point it is your duty to free the entry->entry string, and since you didn't free before loading entry again, there is a leak.  but if you use copy=false, the vorbiscomment will take ownership of the string and there is no leak.

see also http://flac.sourceforge.net/api/group__fla...bject.html#ga23