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

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavfilter/signature_lookup.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@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);
    }
}

> +}
> +
>  static void fill_l1distlut(uint8_t lut[])
>  {
>      int i, j, tmp_i, tmp_j,count;
> @@ -520,16 +529,6 @@ static MatchingInfo evaluate_parameters(AVFilterContext *ctx, SignatureContext *
>      return bestmatch;
>  }
>  
> -static void sll_free(MatchingInfo *sll)
> -{
> -    void *tmp;
> -    while (sll) {
> -        tmp = sll;
> -        sll = sll->next;
> -        av_freep(&tmp);
> -    }
> -}
> -
>  static MatchingInfo lookup_signatures(AVFilterContext *ctx, SignatureContext *sc, StreamContext *first, StreamContext *second, int mode)
>  {
>      CoarseSignature *cs, *cs2;
> @@ -572,7 +571,7 @@ static MatchingInfo lookup_signatures(AVFilterContext *ctx, SignatureContext *sc
>                     "ratio %f, offset %d, score %d, %d frames matching\n",
>                     bestmatch.first->index, bestmatch.second->index,
>                     bestmatch.framerateratio, bestmatch.offset, bestmatch.score, bestmatch.matchframes);
> -            sll_free(infos);
> +            sll_free(&infos);
>          }
>      } while (find_next_coarsecandidate(sc, second->coarsesiglist, &cs, &cs2, 0) && !bestmatch.whole);
>      return bestmatch;

On Mon, Feb 05, 2024 at 12:51:57PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@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 Niedermayer:
> On Mon, Feb 05, 2024 at 12:51:57PM +0100, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Signed-off-by: Michael Niedermayer <michael@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
> 

Your code indeed resets the pointer; it overwrites the pointer once per
loop iteration and so sets it to NULL in the last iteration. I somehow
overlooked that.
I actually consider your code more complex (my code resets the original
pointer and directly traverses the list, your code does the same, but in
between it overwrites the original pointer to store the next pointer
instead of using a simple stack variable for this purpose).
Apply as you wish.

> 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

sll_free_n2() is not recursive.

> 
> -----------
> 
> #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);
> 
> }
>

On Tue, Feb 06, 2024 at 11:36:13AM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Mon, Feb 05, 2024 at 12:51:57PM +0100, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >>> Signed-off-by: Michael Niedermayer <michael@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
> > 
> 
> Your code indeed resets the pointer; it overwrites the pointer once per
> loop iteration and so sets it to NULL in the last iteration. I somehow
> overlooked that.
> I actually consider your code more complex (my code resets the original
> pointer and directly traverses the list, your code does the same, but in
> between it overwrites the original pointer to store the next pointer
> instead of using a simple stack variable for this purpose).

> Apply as you wish.

ok


[...]
> sll_free_n2() is not recursive.

the function is cursed, noone can implement it without bugs

thx

[...]