Re: [PATCH] lib: gui: png_pico: fix use-after-free and double-free in png_open

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello,

Thanks for the fix.

On 6/2/26 4:24 AM, Johannes Schneider wrote:
> From: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
> 
> png_alloc_free_all() frees all picopng-internal allocations, including
> the image->data buffer.  The previous code stored a pointer to this
> buffer in img->data and called png_alloc_free_all() — leaving img->data
> as a dangling pointer.  The subsequent png_close()'s free(img->data)
> then performed a double-free on already-freed memory, causing a crash or
> heap corruption when displaying the boot logo.
> 
> Fix by copying the decoded pixel data into a fresh malloc buffer before
> calling png_alloc_free_all().  png_close() correctly frees this copy.

Never ceases to amaze how long memory corruption can go unnoticed..

> -	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
> +	/*
> +	 * Copy decoded pixels to a stable buffer before png_alloc_free_all()
> +	 * frees the picopng internal allocations (including image->data).
> +	 * Without this copy, img->data would be a dangling pointer and
> +	 * png_close()'s free(img->data) would be a double-free.
> +	 */
> +	imgsize = png_info->width * png_info->height * 4;
> +	imgcopy = malloc(imgsize);
> +	if (!imgcopy) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	memcpy(imgcopy, png_info->image->data, imgsize);
>  
>  	png_alloc_free_all();
>  
> +	img->data = imgcopy;
> +
> +	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
> +

I would prefer avoiding the memory copy here. My suggestion would be adding
(untested):

void *png_alloc_detach(void *addr)
{
        for (png_alloc_node_t *node = png_alloc_tail; node; node = node->prev) {
                if (node->addr == addr) {
                        png_alloc_remove_node(node);
                        return addr;
                }
        }

        return NULL;
}

and then a single line change in png_open:

-	img->data = png_info->image->data
+	img->data = png_alloc_detach(png_info->image->data);

What do you think?

Cheers,
Ahmad

>  	return img;
>  err:
>  	png_alloc_free_all();

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |