Segmentation Fault in PackPixels (setWindowIcon)

[TrevorCash]

Hi All,

I am testing a program on ubuntu 22 that is working on windows and mac, But I am hitting a segfault in the Magnum::Platform::packPixels() function. It is copying an imported image with size 417x417. I am not sure on the possible reasons of this segfault?

Any Tips/ Help Appreciated,
-Trevor

[mosra]

Hmm, I'm looking at the code on my side and don't really see what could be wrong. Do you know if it's a segfault due to a read or due to a write? I'm assuming it's a debug build and you don't get any assertions from the code (as otherwise it'd yell at you if anything in that code performs out-of-bounds access).

The image size, 417x417, is quite interesting for a window icon tho. How does the code that loads it look like? Are there other images passed, or just one?

[TrevorCash]

Here is the call down one on the stack, it is a debug build. It is probably something simple I am missing - I did notice that the stride in the out array view is negative, is this normal?

[mosra]

Negative stride is fine, that just iterates backwards (and zero stride then iterates over the same place in memory over and over). Blog post with more info if you're interested. Here it's negative because the images need to be flipped from origin bottm left to origin top left.

I know what happens on my side, I don't know anything about your part of the code. Are the images coming from an importer? Or are they custom constructed? How are you passing them? Are you sure the image memory doesn't get out of scope before the function enters?

[TrevorCash]

Hi Mosra,
Its a Debug build on GCC 11.4

The images are coming from AnyImageImporter. I first load the data from the file using some file reads (data is cached and always valid) and then use AnyImageImporter to convert to image. I store all image data in cache as well.

imageAssetEntry getImageAsset(std::string resourceImageFileName)
        {
            if (imageAssetLookupMap.count(resourceImageFileName) > 0)
                return imageAssetLookupMap[resourceImageFileName];


            auto manager = ControlSingleton::GetInstance()->zipResourceManager;
            size_t size;
            auto data = manager->AccessResource(resourceImageFileName, size);

            
            
            Magnum::Containers::Pointer<Magnum::Trade::AbstractImporter> importer =
                pluginManager.loadAndInstantiate("AnyImageImporter");

            if(importer == nullptr){
                throwLoggerError(spdlog::default_logger(), "Unable To Load AnyImageImporter PLugin.");
            }

            Corrade::Containers::ArrayView<char> dataView{ data, size };

            bool s = importer->openData(dataView);
            
            auto image = importer->image2D(0);
            CORRADE_INTERNAL_ASSERT(image);

            Magnum::ImageView2D someView(*image);
            auto newTexture = std::make_shared<Magnum::GL::Texture2D>();
            auto newImageView = std::make_shared < Magnum::ImageView2D>(*image);

            imageAssetEntry entry;
            entry.texture = newTexture;
            entry.image = newImageView;
            imageAssetLookupMap.insert({ resourceImageFileName, entry });

            newTexture->setWrapping(Magnum::GL::SamplerWrapping::ClampToEdge)
                .setMagnificationFilter(Magnum::GL::SamplerFilter::Linear)
                .setMinificationFilter(Magnum::GL::SamplerFilter::Linear)
                .setStorage(1, Magnum::GL::textureFormat(image->format()), image->size())
                .setSubImage(0, {}, *image);

            return imageAssetLookupMap[resourceImageFileName];

        }

In Outer scope I call this:

            auto iconImage = controlObjects->getImageAsset("product-icon.png").image;
            
            setWindowIcon(*iconImage.get());

iconImage is a valid shared_ptr to image.

[mosra]

Thanks for the code!

Yeah, the image view is the root cause. The newImageView is a non-owning view on the data from image, so as soon as image goes out of scope, the view points to freed memory, and unfortunately shared pointers won't help with anything there, as it's too late at that point.

What you need to do instead is (for example) storing an actual Trade::ImageData2D, or a shared pointer of it, in the imageAssetEntry.image. Something like

            auto newImage = std::make_shared <Magnum::Trade::ImageData2D>(std::move(*image));

            imageAssetEntry entry;
            entry.texture = newTexture;
            // assuming `image` is std::shared_ptr<Magnum::Trade::ImageData2D>`
            entry.image = newImage;
            
            ...
            
            // and here also `newImage`, as `image` is now moved out
            newTexture.setSubImage(0, {}, *newImage);

[TrevorCash]

Thanks for the help! Very obvious now that you pointed it out. The naming makes alot of sense. I made the changes and things are working (no idea why msvc was letting this through though)

[mosra]

I guess you just got lucky that the memory didn't get reclaimed by the OS yet. Could happen on Linux or Mac as well, especially with smaller allocations it's quite common that this "works" 90% of the time and crashes are happening only seldom.

If I can recommend anything, building with -fsanitize=address can discover these issues very quickly, point you directly to where given piece of memory was allocated, where it was freed too soon, and if it's "just" an out-of-bounds access also how much out of bounds it was. Works on GCC, Clang, and should work with recent MSVC 2022 as well I think.