diff mbox series

[FFmpeg-devel] avformat/assenc: fix incorrect copy of null terminator

Message ID 20230118143113.324703-2-tim@ngus.net
State New
Headers show
Series [FFmpeg-devel] avformat/assenc: fix incorrect copy of null terminator | expand

Commit Message

Tim Angus Jan. 18, 2023, 2:31 p.m. UTC
When writing a subtitle SSA/ASS subtitle file, the
AVCodecParameters::extradata buffer is written directly to the output,
potentially including a null terminating character, which is sometimes
present. The result is the output having a null character in the middle;
this is addressed here by avoiding copying it.

Signed-off-by: Tim Angus <tim@ngus.net>
---
 libavformat/assenc.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

zhilizhao(赵志立) Jan. 31, 2023, 12:37 p.m. UTC | #1
> On Jan 18, 2023, at 22:31, Tim Angus <tim@ngus.net> wrote:
> 
> When writing a subtitle SSA/ASS subtitle file, the
> AVCodecParameters::extradata buffer is written directly to the output,
> potentially including a null terminating character, which is sometimes
> present. The result is the output having a null character in the middle;
> this is addressed here by avoiding copying it.
> 
> Signed-off-by: Tim Angus <tim@ngus.net>
> ---
> libavformat/assenc.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/assenc.c b/libavformat/assenc.c
> index 1600f0a02b..5e74b84575 100644
> --- a/libavformat/assenc.c
> +++ b/libavformat/assenc.c
> @@ -69,6 +69,11 @@ static int write_header(AVFormatContext *s)
>                 ass->trailer = trailer;
>         }
> 
> +        /* extradata may or may not be null terminated; in the case where
> +         * it is, avoid copying a null into the middle of the buffer */
> +        while (header_size > 0 && par->extradata[header_size - 1] == '\0')
> +            header_size--;
> +

The comment is misleading. extradata is always null terminated, although
those paddings don’t count in extradata_size.

>         avio_write(s->pb, par->extradata, header_size);
>         if (par->extradata[header_size - 1] != '\n')
>             avio_write(s->pb, "\r\n", 2);
> -- 
> 2.25.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".
Tim Angus Jan. 31, 2023, 1:37 p.m. UTC | #2
On 31/01/2023 12:37, "zhilizhao(赵志立)" wrote:
>> +        /* extradata may or may not be null terminated; in the case where
>> +         * it is, avoid copying a null into the middle of the buffer */
>> +        while (header_size > 0 && par->extradata[header_size - 1] == '\0')
>> +            header_size--;
>> +
> The comment is misleading. extradata is always null terminated, although
> those paddings don’t count in extradata_size.

That's a bit pedantic, but I take your point. "The contents of extradata 
may or may..." would be better. Following some discussion on IRC, I've 
actually submitted another patch that solves the problem in a different 
way, but I don't think anyone has looked at it yet...

http://ffmpeg.org/pipermail/ffmpeg-devel/2023-January/306017.html
diff mbox series

Patch

diff --git a/libavformat/assenc.c b/libavformat/assenc.c
index 1600f0a02b..5e74b84575 100644
--- a/libavformat/assenc.c
+++ b/libavformat/assenc.c
@@ -69,6 +69,11 @@  static int write_header(AVFormatContext *s)
                 ass->trailer = trailer;
         }
 
+        /* extradata may or may not be null terminated; in the case where
+         * it is, avoid copying a null into the middle of the buffer */
+        while (header_size > 0 && par->extradata[header_size - 1] == '\0')
+            header_size--;
+
         avio_write(s->pb, par->extradata, header_size);
         if (par->extradata[header_size - 1] != '\n')
             avio_write(s->pb, "\r\n", 2);