Possible memory leak when loading an image.

[Robxley]

Hi Everyone,

First, thanks for hello_imgui—it's a very helpful library for building applications with ImGui.

I have a question regarding the source file image_from_asset.cpp, specifically the function static ImageAbstractPtr _GetCachedImage(const char* assetPath).

At line 72: auto assetData = LoadAssetFileData(assetPath);

Shouldn’t there be a call to FreeAssetFileData after LoadAssetFileData is used? Once the data is transferred to the rendering system (GPU) via concreteImage->_impl_StoreTexture, the data stored in the assetData variable is no longer needed and should be freed using the FreeAssetFileData function.

Here’s the current code (lines 69–84 inside image_from_asset.cpp):

        if (concreteImage != nullptr)
        {
            // Load the image using stbi_load_from_memory
            auto assetData = LoadAssetFileData(assetPath);
            IM_ASSERT(assetData.data != nullptr);
            unsigned char*image_data_rgba = stbi_load_from_memory(
                (unsigned char *)assetData.data, (int)assetData.dataSize,
                &concreteImage->Width, &concreteImage->Height, NULL, 4);
            if (image_data_rgba == NULL)
            {
                IM_ASSERT(false && "ImageDx11: Failed to load image!");
                throw std::runtime_error("ImageAbstract: Failed to load image!");
            }

            concreteImage->_impl_StoreTexture(concreteImage->Width, concreteImage->Height, image_data_rgba);
        }

And here’s the proposed code with the FreeAssetFileData call added:

        if (concreteImage != nullptr)
        {
            // Load the image using stbi_load_from_memory
            auto assetData = LoadAssetFileData(assetPath);
            IM_ASSERT(assetData.data != nullptr);
            unsigned char*image_data_rgba = stbi_load_from_memory(
                (unsigned char *)assetData.data, (int)assetData.dataSize,
                &concreteImage->Width, &concreteImage->Height, NULL, 4);
            if (image_data_rgba == NULL)
            {
                FreeAssetFileData(&assetData);
                IM_ASSERT(false && "ImageDx11: Failed to load image!");
                throw std::runtime_error("ImageAbstract: Failed to load image!");
            }

            concreteImage->_impl_StoreTexture(concreteImage->Width, concreteImage->Height, image_data_rgba);
            FreeAssetFileData(&assetData);
        }

[pthom]

Hello,

Shouldn’t there be a call to FreeAssetFileData after LoadAssetFileData is used?

You are absolutely right. I'm confused that I did not spot it. Many thanks for letting me know! I fixed this.

First, thanks for hello_imgui—it's a very helpful library for building applications with ImGui.

Thank you!

[Robxley]

Hi,

I reviewed your code fixes, but I believe there are still a few issues.

Here’s your updated code:

if (concreteImage != nullptr)
{
    // Load the image using stbi_load_from_memory
    auto assetData = LoadAssetFileData(assetPath);
    IM_ASSERT(assetData.data != nullptr);
    unsigned char* image_data_rgba = stbi_load_from_memory(
        (unsigned char *)assetData.data, (int)assetData.dataSize,
        &concreteImage->Width, &concreteImage->Height, NULL, 4);
    if (image_data_rgba == NULL)
    {
        IM_ASSERT(false && "_GetCachedImage: Failed to load image!");
        throw std::runtime_error("_GetCachedImage: Failed to load image!");
    }
    FreeAssetFileData(&assetData);
    stbi_image_free(image_data_rgba);
    concreteImage->_impl_StoreTexture(concreteImage->Width, concreteImage->Height, image_data_rgba);
}

The main issue is with the line stbi_image_free(image_data_rgba). This free function shouldn’t be called before concreteImage->_impl_StoreTexture(concreteImage->Width, concreteImage->Height, image_data_rgba); because the image data image_data_rgba is still needed during the texture storage process. You should free the image data after the texture has been successfully stored.

Additionally, the call to FreeAssetFileData(&assetData) should also occur inside the if block where the exception is thrown. This ensures proper cleanup when an error occurs. For example:

if (concreteImage != nullptr)
{
    // Load the image using stbi_load_from_memory
    auto assetData = LoadAssetFileData(assetPath);
    IM_ASSERT(assetData.data != nullptr);
    unsigned char* image_data_rgba = stbi_load_from_memory(
        (unsigned char *)assetData.data, (int)assetData.dataSize,
        &concreteImage->Width, &concreteImage->Height, NULL, 4);
    if (image_data_rgba == NULL)
    {
        **FreeAssetFileData(&assetData);**
        IM_ASSERT(false && "_GetCachedImage: Failed to load image!");
        throw std::runtime_error("_GetCachedImage: Failed to load image!");
    }

    // Store the texture before freeing resources
    concreteImage->_impl_StoreTexture(concreteImage->Width, concreteImage->Height, image_data_rgba);

    // Free resources after usage
    **FreeAssetFileData(&assetData);**
    stbi_image_free(image_data_rgba);
}

Or to simplify, you can also call FreeAssetFileData(&assetData) right after loading the asset data with stbi_load_from_memory (if stbi_load_from_memory performs a copy and doesn't make a memory mapping on assetData. I doesn't know the stb_image lib), which avoids repeating the line in multiple places:

if (concreteImage != nullptr)
{
    // Load the image using stbi_load_from_memory
    auto assetData = LoadAssetFileData(assetPath);
    IM_ASSERT(assetData.data != nullptr);
    unsigned char* image_data_rgba = stbi_load_from_memory(
        (unsigned char *)assetData.data, (int)assetData.dataSize,
        &concreteImage->Width, &concreteImage->Height, NULL, 4);

    // Free assert after usage inside stbi_load_from_memory
    FreeAssetFileData(&assetData);
    if (image_data_rgba == NULL)
    {
        IM_ASSERT(false && "_GetCachedImage: Failed to load image!");
        throw std::runtime_error("_GetCachedImage: Failed to load image!");
    }

    // Store the texture before freeing resources
    concreteImage->_impl_StoreTexture(concreteImage->Width, concreteImage->Height, image_data_rgba);

    // Free resources after texture storage
    stbi_image_free(image_data_rgba); 
}

By away, well done for the fix by calling stbi_image_free(image_data_rgba);. i saw for FreeAssetFileData but I forget with one in my first code fix.

Bonne journée :D

[pthom]

Many thanks for your vigilance, I have been too fast in my previous commit.

(I do have to maintain several Open Source projects at the same time and it is not always easy to switch context from one to another).

A+, Merci !

[pthom]

PS: You are mentioned in the commit

[Robxley]

Many thanks for your vigilance, I have been too fast in my previous commit. I do have to maintain several Open Source projects at the same time and it is not always easy to switch context from one to another.

I know what it is. 👍 Once again, thanks for your time on these open source projects (like hello_imgui).

PS: You are mentioned in the commit

😀