[FFmpeg-devel] [PATCH 1/2] avfilter/signature_lookup: dont leave uncleared pointers in sll_free()

[Michael Niedermayer]

On Mon, Feb 05, 2024 at 12:51:57PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> >  libavfilter/signature_lookup.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/libavfilter/signature_lookup.c b/libavfilter/signature_lookup.c
> > index 86dd0c66754..52a97e1bc7e 100644
> > --- a/libavfilter/signature_lookup.c
> > +++ b/libavfilter/signature_lookup.c
> > @@ -37,6 +37,15 @@
> >  #define STATUS_END_REACHED 1
> >  #define STATUS_BEGIN_REACHED 2
> >  
> > +static void sll_free(MatchingInfo **sll)
> > +{
> > +    while (*sll) {
> > +        MatchingInfo *tmp = *sll;
> > +        *sll = (*sll)->next;
> > +        av_free(tmp);
> > +    }
> 
> This does not clear the pointers at all. This does (and avoids
> indirections).
> 
> static void sll_free(MatchingInfo **sllp)
> {
>     MatchingInfo *sll = *sllp;
> 
>     *sllp = NULL;
>     while (sll) {
>         MatchingInfo *tmp = sll;
>         sll = sll->next;
>         av_free(tmp);
>     }
> }

I tried it with code below, but your code is not different from mine in behavior just more complex

output:
(nil) 0x560e8daad2c0 (nil)
vs.
(nil) 0x557ae6e472c0 (nil)

sll_free_n2() is simpler and will clear all, the reason i did not
propose it, is its recursive and can hit stack space limits in principle
sll_free_n3() and sll_free_n4() are other options that will clear all
but maybe every choice contains bugs, i didnt really test them with more than one testcase

-----------

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)

static void av_free(void *ptr)
{
    free(ptr);
}

static void av_freep(void *arg)
{
    void *val;

    memcpy(&val, arg, sizeof(val));
    memcpy(arg, &(void *){ NULL }, sizeof(val));
    av_free(val);
}


typedef struct MatchingInfo {
    struct MatchingInfo *next;
} MatchingInfo;


static void sll_free_n(MatchingInfo **sll)
{
    while (*sll) {
        MatchingInfo *tmp = *sll;
        *sll = (*sll)->next;
        av_free(tmp);
    }
}

static void sll_free_n2(MatchingInfo **sll)
{
    if (*sll)
        sll_free_n(&(*sll)->next);
    av_freep(sll);
}

static void sll_free_n3(MatchingInfo **sll)
{
    while (*sll) {
        MatchingInfo *tmp = *sll;
        *sll = tmp->next;
        tmp->next = NULL;
        av_free(tmp);
    }
}

static void sll_free_n4(MatchingInfo **sll)
{
    MatchingInfo *tmp = NULL;
    while (*sll) {
        FFSWAP(MatchingInfo *, tmp, (*sll)->next);
        av_freep(sll);
        FFSWAP(MatchingInfo *, tmp, *sll);
    }
}

static void sll_free_r(MatchingInfo **sllp)
{
    MatchingInfo *sll = *sllp;

    *sllp = NULL;
    while (sll) {
        MatchingInfo *tmp = sll;
        sll = sll->next;
        av_free(tmp);
    }
}

main() {
    MatchingInfo *mi, *m1, *m2;

    m1 = mi = malloc(sizeof(MatchingInfo));
    m2 = mi->next = malloc(sizeof(MatchingInfo));
    m2->next= NULL;

    sll_free_r(&mi);

    printf("%p %p %p\n", mi, m1->next, m2->next);

}

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240206/2afc807d/attachment.sig>