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

[Andreas Rheinhardt]

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
> 

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