diff mbox series

[FFmpeg-devel] avcodec/bsf: switch to av_get_token to parse bsf list string

Message ID 20210703114222.5684-1-ffmpeg@gyani.pro
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/bsf: switch to av_get_token to parse bsf list string
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Gyan Doshi July 3, 2021, 11:42 a.m. UTC
The recently added setts bsf makes use of the eval API whose
expressions can contain commas. The existing parsing in
av_bsf_list_parse_str() uses av_strtok to naively split
the string at commas, thus preventing the use of setts filter
with expressions containing commas.
---
 libavcodec/bsf.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

James Almer July 3, 2021, 2:17 p.m. UTC | #1
On 7/3/2021 8:42 AM, Gyan Doshi wrote:
> The recently added setts bsf makes use of the eval API whose
> expressions can contain commas. The existing parsing in
> av_bsf_list_parse_str() uses av_strtok to naively split
> the string at commas, thus preventing the use of setts filter
> with expressions containing commas.
> ---
>   libavcodec/bsf.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 9d67ea5395..726911785d 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -520,7 +520,8 @@ static int bsf_parse_single(char *str, AVBSFList *bsf_lst)
>   int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
>   {
>       AVBSFList *lst;
> -    char *bsf_str, *buf, *dup, *saveptr;
> +    char *bsf_str, *dup;
> +    const char *buf;
>       int ret;
>   
>       if (!str)
> @@ -530,18 +531,18 @@ int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
>       if (!lst)
>           return AVERROR(ENOMEM);
>   
> -    if (!(dup = buf = av_strdup(str))) {
> +    if (!(buf = dup = av_strdup(str))) {

Is this av_strdup() even necessary? You could let av_get_token() update 
str just fine and remove both buf and dup. Or maybe just use a copy of 
the pointer in buf.

>           ret = AVERROR(ENOMEM);
>           goto end;
>       }
>   
> -    while (bsf_str = av_strtok(buf, ",", &saveptr)) {
> +    do {
> +        bsf_str = av_get_token(&buf, ",");

You can reduce the scope of bsf_str now, so declare it here.

>           ret = bsf_parse_single(bsf_str, lst);
> +        av_free(bsf_str);
>           if (ret < 0)
>               goto end;
> -
> -        buf = NULL;
> -    }
> +    } while (*buf == ',' && buf++);

You can simplify this into while (*++buf) or (*++str) if you remove the 
av_strdup(). It will also preserve the existing behavior of discarding 
the last comma in the string if it's the last character, instead of 
aborting with EINVAL.

>   
>       ret = av_bsf_list_finalize(&lst, bsf_lst);
>   end:
>
Gyan Doshi July 3, 2021, 3:48 p.m. UTC | #2
On 2021-07-03 19:47, James Almer wrote:
> On 7/3/2021 8:42 AM, Gyan Doshi wrote:
>> The recently added setts bsf makes use of the eval API whose
>> expressions can contain commas. The existing parsing in
>> av_bsf_list_parse_str() uses av_strtok to naively split
>> the string at commas, thus preventing the use of setts filter
>> with expressions containing commas.
>> ---
>>   libavcodec/bsf.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>> index 9d67ea5395..726911785d 100644
>> --- a/libavcodec/bsf.c
>> +++ b/libavcodec/bsf.c
>> @@ -520,7 +520,8 @@ static int bsf_parse_single(char *str, AVBSFList 
>> *bsf_lst)
>>   int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
>>   {
>>       AVBSFList *lst;
>> -    char *bsf_str, *buf, *dup, *saveptr;
>> +    char *bsf_str, *dup;
>> +    const char *buf;
>>       int ret;
>>         if (!str)
>> @@ -530,18 +531,18 @@ int av_bsf_list_parse_str(const char *str, 
>> AVBSFContext **bsf_lst)
>>       if (!lst)
>>           return AVERROR(ENOMEM);
>>   -    if (!(dup = buf = av_strdup(str))) {
>> +    if (!(buf = dup = av_strdup(str))) {
>
> Is this av_strdup() even necessary? You could let av_get_token() 
> update str just fine and remove both buf and dup. Or maybe just use a 
> copy of the pointer in buf.
>
>>           ret = AVERROR(ENOMEM);
>>           goto end;
>>       }
>>   -    while (bsf_str = av_strtok(buf, ",", &saveptr)) {
>> +    do {
>> +        bsf_str = av_get_token(&buf, ",");
>
> You can reduce the scope of bsf_str now, so declare it here.
>
>>           ret = bsf_parse_single(bsf_str, lst);
>> +        av_free(bsf_str);
>>           if (ret < 0)
>>               goto end;
>> -
>> -        buf = NULL;
>> -    }
>> +    } while (*buf == ',' && buf++);
>
> You can simplify this into while (*++buf) or (*++str) if you remove 
> the av_strdup().

Won't this go out of bounds at the end?


> It will also preserve the existing behavior of discarding the last 
> comma in the string if it's the last character, instead of aborting 
> with EINVAL.
>
>>         ret = av_bsf_list_finalize(&lst, bsf_lst);
>>   end:
>>
>
> _______________________________________________
> 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".
James Almer July 3, 2021, 4:08 p.m. UTC | #3
On 7/3/2021 12:48 PM, Gyan Doshi wrote:
> 
> 
> On 2021-07-03 19:47, James Almer wrote:
>> On 7/3/2021 8:42 AM, Gyan Doshi wrote:
>>> The recently added setts bsf makes use of the eval API whose
>>> expressions can contain commas. The existing parsing in
>>> av_bsf_list_parse_str() uses av_strtok to naively split
>>> the string at commas, thus preventing the use of setts filter
>>> with expressions containing commas.
>>> ---
>>>   libavcodec/bsf.c | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>> index 9d67ea5395..726911785d 100644
>>> --- a/libavcodec/bsf.c
>>> +++ b/libavcodec/bsf.c
>>> @@ -520,7 +520,8 @@ static int bsf_parse_single(char *str, AVBSFList 
>>> *bsf_lst)
>>>   int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
>>>   {
>>>       AVBSFList *lst;
>>> -    char *bsf_str, *buf, *dup, *saveptr;
>>> +    char *bsf_str, *dup;
>>> +    const char *buf;
>>>       int ret;
>>>         if (!str)
>>> @@ -530,18 +531,18 @@ int av_bsf_list_parse_str(const char *str, 
>>> AVBSFContext **bsf_lst)
>>>       if (!lst)
>>>           return AVERROR(ENOMEM);
>>>   -    if (!(dup = buf = av_strdup(str))) {
>>> +    if (!(buf = dup = av_strdup(str))) {
>>
>> Is this av_strdup() even necessary? You could let av_get_token() 
>> update str just fine and remove both buf and dup. Or maybe just use a 
>> copy of the pointer in buf.
>>
>>>           ret = AVERROR(ENOMEM);
>>>           goto end;
>>>       }
>>>   -    while (bsf_str = av_strtok(buf, ",", &saveptr)) {
>>> +    do {
>>> +        bsf_str = av_get_token(&buf, ",");
>>
>> You can reduce the scope of bsf_str now, so declare it here.
>>
>>>           ret = bsf_parse_single(bsf_str, lst);
>>> +        av_free(bsf_str);
>>>           if (ret < 0)
>>>               goto end;
>>> -
>>> -        buf = NULL;
>>> -    }
>>> +    } while (*buf == ',' && buf++);
>>
>> You can simplify this into while (*++buf) or (*++str) if you remove 
>> the av_strdup().
> 
> Won't this go out of bounds at the end?

Yeah, good catch. Change it to while (*buf && *++buf) then, which will 
also preserve the existing behavior.

> 
> 
>> It will also preserve the existing behavior of discarding the last 
>> comma in the string if it's the last character, instead of aborting 
>> with EINVAL.
>>
>>>         ret = av_bsf_list_finalize(&lst, bsf_lst);
>>>   end:
>>>
>>
>> _______________________________________________
>> 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 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".
diff mbox series

Patch

diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index 9d67ea5395..726911785d 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -520,7 +520,8 @@  static int bsf_parse_single(char *str, AVBSFList *bsf_lst)
 int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
 {
     AVBSFList *lst;
-    char *bsf_str, *buf, *dup, *saveptr;
+    char *bsf_str, *dup;
+    const char *buf;
     int ret;
 
     if (!str)
@@ -530,18 +531,18 @@  int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
     if (!lst)
         return AVERROR(ENOMEM);
 
-    if (!(dup = buf = av_strdup(str))) {
+    if (!(buf = dup = av_strdup(str))) {
         ret = AVERROR(ENOMEM);
         goto end;
     }
 
-    while (bsf_str = av_strtok(buf, ",", &saveptr)) {
+    do {
+        bsf_str = av_get_token(&buf, ",");
         ret = bsf_parse_single(bsf_str, lst);
+        av_free(bsf_str);
         if (ret < 0)
             goto end;
-
-        buf = NULL;
-    }
+    } while (*buf == ',' && buf++);
 
     ret = av_bsf_list_finalize(&lst, bsf_lst);
 end: