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

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

[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);
+    }
+}
+
 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;
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avfilter/signature_lookup: Do not dereference NULL pointers after malloc failure

[Michael Niedermayer]

Fixes: CID 1403229 Dereference after null check

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

diff --git a/libavfilter/signature_lookup.c b/libavfilter/signature_lookup.c
index 52a97e1bc7e..0c724456e24 100644
--- a/libavfilter/signature_lookup.c
+++ b/libavfilter/signature_lookup.c
@@ -298,6 +298,11 @@ static MatchingInfo* get_matching_parameters(AVFilterContext *ctx, SignatureCont
                         if (!c->next)
                             av_log(ctx, AV_LOG_FATAL, "Could not allocate memory");
                         c = c->next;
+
+                    }
+                    if (!c) {
+                        sll_free(&cands);
+                        goto error;
                     }
                     c->framerateratio = (i+1.0) / 30;
                     c->score = hspace[i][j].score;
@@ -314,6 +319,7 @@ static MatchingInfo* get_matching_parameters(AVFilterContext *ctx, SignatureCont
             }
         }
     }
+    error:
     for (i = 0; i < MAX_FRAMERATE; i++) {
         av_freep(&hspace[i]);
     }
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avfilter/signature_lookup: Do not dereference NULL pointers after malloc failure

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 524 bytes --]

On Mon, Feb 05, 2024 at 12:44:59PM +0100, Michael Niedermayer wrote:
> Fixes: CID 1403229 Dereference after null check
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavfilter/signature_lookup.c | 6 ++++++
>  1 file changed, 6 insertions(+)

will apply

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

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

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;

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3495 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@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);
> 
> }
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2188 bytes --]

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

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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".