[FFmpeg-devel] [PATCH 0/1] Fix off-by-one in ASS subtitle encoder

[FFmpeg-devel] [PATCH 0/1] Fix off-by-one in ASS subtitle encoder

[Tim Angus]

When writing a subtitle SSA/ASS subtitle file, the
AVCodecParameters::extradata buffer is written directly to the output,
including the null terminating character of the buffer. This appears to
be a simple off by one bug, which is fixed by the subsequent patch.

Tim Angus (1):
  avformat/assenc: fix incorrect copy of null terminator

 libavformat/assenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Tim Angus]

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

diff --git a/libavformat/assenc.c b/libavformat/assenc.c
index 1600f0a02b..07b6e3a171 100644
--- a/libavformat/assenc.c
+++ b/libavformat/assenc.c
@@ -69,7 +69,7 @@ static int write_header(AVFormatContext *s)
                 ass->trailer = trailer;
         }
 
-        avio_write(s->pb, par->extradata, header_size);
+        avio_write(s->pb, par->extradata, header_size - 1);
         if (par->extradata[header_size - 1] != '\n')
             avio_write(s->pb, "\r\n", 2);
         ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]");
-- 
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".

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

[Andreas Rheinhardt]

Tim Angus:
> Signed-off-by: Tim Angus <tim@ngus.net>
> ---
>  libavformat/assenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/assenc.c b/libavformat/assenc.c
> index 1600f0a02b..07b6e3a171 100644
> --- a/libavformat/assenc.c
> +++ b/libavformat/assenc.c
> @@ -69,7 +69,7 @@ static int write_header(AVFormatContext *s)
>                  ass->trailer = trailer;
>          }
>  
> -        avio_write(s->pb, par->extradata, header_size);
> +        avio_write(s->pb, par->extradata, header_size - 1);
>          if (par->extradata[header_size - 1] != '\n')
>              avio_write(s->pb, "\r\n", 2);
>          ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]");

1. The rationale for the patch (that you mentioned in the cover letter)
should be part of the commit message.
2. Did you run FATE with your patch? This should actually change the
output of some tests.
3. The '\0' is not supposed to be accounted for in extradata_size;
extradata is supposed to be padded with AV_INPUT_BUFFER_PADDING_SIZE
zero bytes, the first of which also acts as trailing zero for formats
for which extradata is a C-string. (And anyway: There are cases where
header_size does not coincide with extradata_size, yet you are also
changing them.)

- Andreas

_______________________________________________
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/1] avformat/assenc: fix incorrect copy of null terminator

[Tim Angus]

On 05/01/2023 21:11, Andreas Rheinhardt wrote:
> Tim Angus:
>> Signed-off-by: Tim Angus <tim@ngus.net>
>> ---
>>   libavformat/assenc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/assenc.c b/libavformat/assenc.c
>> index 1600f0a02b..07b6e3a171 100644
>> --- a/libavformat/assenc.c
>> +++ b/libavformat/assenc.c
>> @@ -69,7 +69,7 @@ static int write_header(AVFormatContext *s)
>>                   ass->trailer = trailer;
>>           }
>>   
>> -        avio_write(s->pb, par->extradata, header_size);
>> +        avio_write(s->pb, par->extradata, header_size - 1);
>>           if (par->extradata[header_size - 1] != '\n')
>>               avio_write(s->pb, "\r\n", 2);
>>           ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]");
> 1. The rationale for the patch (that you mentioned in the cover letter)
> should be part of the commit message.
Fair enough.
> 2. Did you run FATE with your patch? This should actually change the
> output of some tests.
Yes I did; no failures locally, though I see there are failures in this 
"patchwork" thingy, presumably that is running extra tests?
> 3. The '\0' is not supposed to be accounted for in extradata_size;
> extradata is supposed to be padded with AV_INPUT_BUFFER_PADDING_SIZE
> zero bytes, the first of which also acts as trailing zero for formats
> for which extradata is a C-string. (And anyway: There are cases where
> header_size does not coincide with extradata_size, yet you are also
> changing them.)
>
Having read this, I did a bit more digging and it appears as though the 
source of the extra nul may actually be embedded in the file I was 
having trouble with in the first place itself, so my patch is probably 
prematurely submitted, sorry. mkvtoolnix seems to extract the subtitle 
file with no trouble so it's not really clear where the fault lies. I'm 
probably better submitting a bug to the tracker rather than trying to 
fix it myself, tbh.

(Out of interest, what is the policy WRT to broken files? You could make 
a reasonable case that the encoder should filter the extra nul, assuming 
of course that the extra nul is indeed extra and there is no ffmpeg bug.)
_______________________________________________
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/1] avformat/assenc: fix incorrect copy of null terminator

[Andreas Rheinhardt]

Tim Angus:
> 
> 
> On 05/01/2023 21:11, Andreas Rheinhardt wrote:
>> Tim Angus:
>>> Signed-off-by: Tim Angus <tim@ngus.net>
>>> ---
>>>   libavformat/assenc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/assenc.c b/libavformat/assenc.c
>>> index 1600f0a02b..07b6e3a171 100644
>>> --- a/libavformat/assenc.c
>>> +++ b/libavformat/assenc.c
>>> @@ -69,7 +69,7 @@ static int write_header(AVFormatContext *s)
>>>                   ass->trailer = trailer;
>>>           }
>>>   -        avio_write(s->pb, par->extradata, header_size);
>>> +        avio_write(s->pb, par->extradata, header_size - 1);
>>>           if (par->extradata[header_size - 1] != '\n')
>>>               avio_write(s->pb, "\r\n", 2);
>>>           ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]");
>> 1. The rationale for the patch (that you mentioned in the cover letter)
>> should be part of the commit message.
> Fair enough.
>> 2. Did you run FATE with your patch? This should actually change the
>> output of some tests.
> Yes I did; no failures locally, though I see there are failures in this
> "patchwork" thingy, presumably that is running extra tests?

Did you run FATE with samples or without?

>> 3. The '\0' is not supposed to be accounted for in extradata_size;
>> extradata is supposed to be padded with AV_INPUT_BUFFER_PADDING_SIZE
>> zero bytes, the first of which also acts as trailing zero for formats
>> for which extradata is a C-string. (And anyway: There are cases where
>> header_size does not coincide with extradata_size, yet you are also
>> changing them.)
>>
> Having read this, I did a bit more digging and it appears as though the
> source of the extra nul may actually be embedded in the file I was
> having trouble with in the first place itself, so my patch is probably
> prematurely submitted, sorry. mkvtoolnix seems to extract the subtitle
> file with no trouble so it's not really clear where the fault lies. I'm
> probably better submitting a bug to the tracker rather than trying to
> fix it myself, tbh.
> 
> (Out of interest, what is the policy WRT to broken files? You could make
> a reasonable case that the encoder should filter the extra nul, assuming
> of course that the extra nul is indeed extra and there is no ffmpeg bug.)

_______________________________________________
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/1] avformat/assenc: fix incorrect copy of null terminator

[Tim Angus]

On 05/01/2023 22:03, Andreas Rheinhardt wrote:
>> Yes I did; no failures locally, though I see there are failures in this
>> "patchwork" thingy, presumably that is running extra tests?
> Did you run FATE with samples or without?
No idea, I just ran "make fate" as per the submission checklist.
_______________________________________________
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".