diff mbox series

[FFmpeg-devel] avformat/concat: finalize the AVBprint buffer immediately

Message ID 20210726130506.374-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/concat: finalize the AVBprint buffer immediately
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

James Almer July 26, 2021, 1:05 p.m. UTC
Don't attempt to read its contents in place.
Fixes invalid reads when run under Valgrind.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/concat.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Marton Balint July 26, 2021, 9:49 p.m. UTC | #1
On Mon, 26 Jul 2021, James Almer wrote:

> Don't attempt to read its contents in place.
> Fixes invalid reads when run under Valgrind.

As far as I remember AVBPrint buffer CAN be read in place by design, 
zero terminator is always guaranteed, not only after finalizing. So this 
should not be needed. What is causing the invalid reads exactly?

Thanks,
Marton

>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> libavformat/concat.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/concat.c b/libavformat/concat.c
> index aec1f52d8e..64ac03e1d8 100644
> --- a/libavformat/concat.c
> +++ b/libavformat/concat.c
> @@ -211,6 +211,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
>     struct concat_data *data = h->priv_data;
>     AVIOContext *in = NULL;
>     const char *cursor;
> +    char *buf;
>     int64_t total_size = 0;
>     unsigned int nodes_size = 0;
>     size_t i = 0;
> @@ -238,7 +239,11 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
>         return err;
>     }
>
> -    cursor = bp.str;
> +    err = av_bprint_finalize(&bp, &buf);
> +    if (err < 0)
> +        return err;
> +
> +    cursor = buf;
>     while (*cursor) {
>         struct concat_nodes *nodes;
>         URLContext *uc;
> @@ -286,7 +291,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
>         data->nodes[i++].size = size;
>         total_size += size;
>     }
> -    av_bprint_finalize(&bp, NULL);
> +    av_free(buf);
>     data->length = i;
>
>     if (err < 0)
> -- 
> 2.32.0
>
> _______________________________________________
> 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".
>
Nicolas George July 26, 2021, 9:54 p.m. UTC | #2
Marton Balint (12021-07-26):
> As far as I remember AVBPrint buffer CAN be read in place by design, zero
> terminator is always guaranteed, not only after finalizing. So this should
> not be needed. What is causing the invalid reads exactly?

I confirm your memory. The documentation states:

 * The string buffer grows as necessary and is always 0-terminated.

And I checked: there is no obvious bug in the avio_read_to_bprint() code
path, it ends with av_bprint_grow() adding the final 0.

Regards,
James Almer July 27, 2021, 2:04 a.m. UTC | #3
On 7/26/2021 6:49 PM, Marton Balint wrote:
> 
> 
> On Mon, 26 Jul 2021, James Almer wrote:
> 
>> Don't attempt to read its contents in place.
>> Fixes invalid reads when run under Valgrind.
> 
> As far as I remember AVBPrint buffer CAN be read in place by design, 
> zero terminator is always guaranteed, not only after finalizing. So this 
> should not be needed. What is causing the invalid reads exactly?
> 
> Thanks,
> Marton

It happens only when you use a URI string that doesn't end with a line 
break. I just noticed that after this patch some invalid reads still 
happen in any case.

I think i found out the culprit, and the following seems to fix it:

> diff --git a/libavformat/concat.c b/libavformat/concat.c
> index aec1f52d8e..94917840c6 100644
> --- a/libavformat/concat.c
> +++ b/libavformat/concat.c
> @@ -251,7 +251,7 @@ static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
>              err = AVERROR(ENOMEM);
>              break;
>          }
> -        cursor++;
> +        if (*cursor) cursor++;
> 
>          if (++len == SIZE_MAX / sizeof(*nodes)) {
>              av_free(node_uri);

Basically, av_get_token() returns a pointer to the terminating 
character, so cursor++ for the next loop when said character was \0 was 
wrong.
Marton Balint July 27, 2021, 8:12 a.m. UTC | #4
On Mon, 26 Jul 2021, James Almer wrote:

> On 7/26/2021 6:49 PM, Marton Balint wrote:
>> 
>> 
>> On Mon, 26 Jul 2021, James Almer wrote:
>> 
>>> Don't attempt to read its contents in place.
>>> Fixes invalid reads when run under Valgrind.
>> 
>> As far as I remember AVBPrint buffer CAN be read in place by design, zero 
>> terminator is always guaranteed, not only after finalizing. So this should 
>> not be needed. What is causing the invalid reads exactly?
>> 
>> Thanks,
>> Marton
>
> It happens only when you use a URI string that doesn't end with a line break. 
> I just noticed that after this patch some invalid reads still happen in any 
> case.
>
> I think i found out the culprit, and the following seems to fix it:
>
>> diff --git a/libavformat/concat.c b/libavformat/concat.c
>> index aec1f52d8e..94917840c6 100644
>> --- a/libavformat/concat.c
>> +++ b/libavformat/concat.c
>> @@ -251,7 +251,7 @@ static av_cold int concatf_open(URLContext *h, const 
>> char *uri, int flags)
>>              err = AVERROR(ENOMEM);
>>              break;
>>          }
>> -        cursor++;
>> +        if (*cursor) cursor++;

Yeah, I suspected the same. So this seems to be the correct patch to 
apply.

Thanks,
Marton

>>
>>          if (++len == SIZE_MAX / sizeof(*nodes)) {
>>              av_free(node_uri);
>
> Basically, av_get_token() returns a pointer to the terminating character, so 
> cursor++ for the next loop when said character was \0 was wrong.
> _______________________________________________
> 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/libavformat/concat.c b/libavformat/concat.c
index aec1f52d8e..64ac03e1d8 100644
--- a/libavformat/concat.c
+++ b/libavformat/concat.c
@@ -211,6 +211,7 @@  static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
     struct concat_data *data = h->priv_data;
     AVIOContext *in = NULL;
     const char *cursor;
+    char *buf;
     int64_t total_size = 0;
     unsigned int nodes_size = 0;
     size_t i = 0;
@@ -238,7 +239,11 @@  static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
         return err;
     }
 
-    cursor = bp.str;
+    err = av_bprint_finalize(&bp, &buf);
+    if (err < 0)
+        return err;
+
+    cursor = buf;
     while (*cursor) {
         struct concat_nodes *nodes;
         URLContext *uc;
@@ -286,7 +291,7 @@  static av_cold int concatf_open(URLContext *h, const char *uri, int flags)
         data->nodes[i++].size = size;
         total_size += size;
     }
-    av_bprint_finalize(&bp, NULL);
+    av_free(buf);
     data->length = i;
 
     if (err < 0)