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

[SCHNEIDER Johannes <johannes.schneider@leica-geosystems.com>]

Hoi Ahmad,

>
>
> Hello,
>
> Thanks for the fix.
>

:-D

> 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?
>

good idea, sending out a v2
(with your 'suggested-by' :-)

>
> Cheers,
> Ahmad
>
> >       return img;
> >  err:
> >       png_alloc_free_all();
>

gruß
Johannes