jarikomppa / soloud

Free, easy, portable audio engine for games
http://soloud-audio.com
Other
1.8k stars 284 forks source link

Speech Valgrind-proof #134

Closed Flix01 closed 8 years ago

Flix01 commented 8 years ago

Hi. I've run Valgrind (a memory leak detector) and I've found that it's happier if we change:

    SpeechInstance::~SpeechInstance(){
               //delete mSample;    // old    
                delete[] mSample;  // new
    }
char * darray::getDataInPos(int aPosition) {
...
char *newdata = (char*)realloc(mData, newsize);
        if (!newdata)
        {
            free(mData);
            mData = NULL;
            mAllocated = mUsed = 0;
            return NULL;
        }
        else memset(newdata,0,newsize); // new
...
}

Not sure that the latter is a real memory leak anyway...

jarikomppa commented 8 years ago

Hm, the last one will wipe anything that might have been in the old buffer after it gets resized, so I'm not quite sure if it's okay.. =) I presume your intention is to clear the newly allocated memory area to zero, which is generally a good idea, which (if I haven't messed anything up here) should be

On Mon, Apr 18, 2016 at 3:53 PM, Flix notifications@github.com wrote:

Hi. I've run Valgrind (a memory leak detector) and I've found that it's happier if we change:

SpeechInstance::~SpeechInstance(){
           //delete mSample;    // old
            delete[] mSample;  // new
}

char * darray::getDataInPos(int aPosition) { ...char newdata = (char)realloc(mData, newsize); if (!newdata) { free(mData); mData = NULL; mAllocated = mUsed = 0; return NULL; } else memset(newdata,0,newsize); // new ... }

Not sure that the latter is a real memory leak anyway...

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/jarikomppa/soloud/issues/134

Flix01 commented 8 years ago

Yes, thank you for the correct fix: Valgrind is OK with the partial memset too (basically Valgrind detected that some uninitialized memory allocated there was triggering an if statement somewhere: so the behaviour was not deterministic or well defined).

P.S. I've found another memory leak when using pthreads.

        ThreadHandle createThread(threadFunction aThreadFunction, void *aParameter)
        {
            soloud_thread_data *d = new soloud_thread_data;
            d->mFunc = aThreadFunction;
            d->mParam = aParameter;

            ThreadHandleData *threadHandle = new ThreadHandleData;   // here: probably it's not released at the end
            pthread_create(&threadHandle->thread, NULL, threadfunc, (void*)d);
            return threadHandle;
        }

Tested with Linux + OpenAL. It does not happen with Linux + SDL2 (maybe it's not using pthreads).

jarikomppa commented 8 years ago

You're right, the thread handles are not collected and release() is not explicitly called, so that's a potential leak. I don't think it's a real one though.. Not 100% certain of that, though.

On Sun, Apr 24, 2016 at 2:30 PM, Flix notifications@github.com wrote:

Yes, thank you for the correct fix: Valgrind is OK with the partial memset too (basically Valgrind detected that some uninitialized memory allocated there was triggering an if statement somewhere: so the behaviour was not deterministic or well defined).

P.S. I've found another memory leak when using pthreads.

    ThreadHandle createThread(threadFunction aThreadFunction, void *aParameter)
    {
        soloud_thread_data *d = new soloud_thread_data;
        d->mFunc = aThreadFunction;
        d->mParam = aParameter;

        ThreadHandleData *threadHandle = new ThreadHandleData;   // here: probably it's not released at the end
        pthread_create(&threadHandle->thread, NULL, threadfunc, (void*)d);
        return threadHandle;
    }

Tested with Linux + OpenAL. It does not happen with Linux + SDL2 (maybe it's not using pthreads).

— You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub https://github.com/jarikomppa/soloud/issues/134#issuecomment-213944038