[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 at 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.)