t-mat / lz4mt

Platform independent, multi-threading implementation of lz4 stream in C++11
Other
199 stars 47 forks source link

Memory Leak is lz4mt.cpp #18

Closed alecmus closed 11 years ago

alecmus commented 11 years ago

There is a memory leak caused by lz4mt.cpp. It is caused by the assignment of dynamic memory in line 471:

auto src = srcBufferPool.alloc();

Each time the loop runs this memory is not freed.

Discovered the memory leak using the CRT Debug Library: _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

Pinpointed the source to the location using Visual Leak Detector (http://vld.codeplex.com/).

Suggested solution:

for(int i = 0;; ++i) {
    auto src = srcBufferPool.alloc();
    auto* srcPtr = src->data();
    const auto srcSize = src->size();
    const auto readSize = ctx->read(srcPtr, static_cast<int>(srcSize));

    if(0 == readSize) {
        break;
    }

    if(singleThread) {
        f(0, src, readSize);
    } else {
        futures.emplace_back(std::async(launch, f, i, src, readSize));
    }
    delete src; /* clear memory */
}

No memory leak detected after this adjustment.

alecmus commented 11 years ago

The remedy I proposed above seems to be flawed. After this adjustment the program crashes "randomly" on compression/decompression cycles.

Anyone who can offer a better solution to this issue?

t-mat commented 11 years ago

Hi alecmus. Thanks for the report. I'll investigate this problem.

t-mat commented 11 years ago

I'm testing following solution.

Quick Solution

Replace this break with following code:

    if(0 == readSize) {
        delete src;  // fix issue #18
        break;
    }

lz4mtDecompress() has same pattern, same memory leak.

To solve this problem, insert delete src; to line 712 and line 720.

Since these breaks handle error case, maybe you've not seen memory leak in lz4mtDecompress().

(1) src is (almost) always deleted.

This BufferPtr is std::unique_ptr. So the src which is passed to f() will be always deleted.

(2) Your proposal will cause random crash (as you've seen).

Firstly, delete src at end of for loop seems good. But src is passed to f() with std::async(), so your proposal may delete src before f() will be done all procedure.

What is not deleted ?

This break makes a bad code path. In this branch, src is not deleted.

t-mat commented 11 years ago
alecmus commented 11 years ago

Thanks for your effort. Hadn't actually noticed this break in 477. After a short look it's clear that is exactly where src is left 'undeleted'. And indeed hadn't noticed the potential leak in lz4mtDecompress(). Also testing the solution you've proposed.