[FFmpeg-devel] [PATCH 0/1] Fix off-by-one in ASS subtitle encoder
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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/1] avformat/assenc: fix incorrect copy of null terminator
Signed-off-by: Tim Angus --- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/1] avformat/assenc: fix incorrect copy of null terminator
On 05/01/2023 21:11, Andreas Rheinhardt wrote: Tim Angus: Signed-off-by: Tim Angus --- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/1] avformat/assenc: fix incorrect copy of null terminator
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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 0/1] Handle ASS format subtitle encoding ambiguity
Some matroska files embed ASS format subtitles. The header for said subtitles include the header for the subtitle stream in the "codec private data" section. It appears to be optional whether or not the last byte of this data is 0, i.e. a null terminator for the string data. Using ffmpeg to extract subtitles for such a file, this header is copied directly to the output file, including the null terminator, if it was present. This results in a file in which there is a null terminator after the header, but preceeding the actual content of the subtitle file. Obviously this is not correct. As a data point, of the ~600 mkvs I have locally, 22 of them have ASS subtitles, and of them 20 include the null terminator, so it doesn't appear to be a rare phenomenon. As another data point, the tool mkvextract from mkvtoolnix avoids the ambiguity by first assuming that the source buffer is *not* null terminated, and then manually adding a (possibly second) null terminator. The buffer is then interpreted as a null terminated string and processed that way. (https://gitlab.com/mbunkus/mkvtoolnix/-/blob/main/src/extract/xtr_textsubs.cpp#L117) My change here simply avoids copying the trailing null terminator(s) present. FATE succeeds as there are no mkvs in the suite that have ASS subtitles embedded. Tim Angus (1): avformat/assenc: fix incorrect copy of null terminator libavformat/assenc.c | 6 ++ 1 file changed, 6 insertions(+) -- 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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] avformat/assenc: fix incorrect copy of null terminator
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 --- 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--; + 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 0/1] Handle ASS format subtitle encoding ambiguity
On 18/01/2023 14:31, Tim Angus wrote: Some matroska files embed ASS format subtitles. The header for said subtitles include the header for the subtitle stream in the "codec private data" section. It appears to be optional whether or not the last byte of this data is 0, i.e. a null terminator for the string data. Using ffmpeg to extract subtitles for such a file, this header is copied directly to the output file, including the null terminator, if it was present. This results in a file in which there is a null terminator after the header, but preceeding the actual content of the subtitle file. Obviously this is not correct. As a data point, of the ~600 mkvs I have locally, 22 of them have ASS subtitles, and of them 20 include the null terminator, so it doesn't appear to be a rare phenomenon. As another data point, the tool mkvextract from mkvtoolnix avoids the ambiguity by first assuming that the source buffer is *not* null terminated, and then manually adding a (possibly second) null terminator. The buffer is then interpreted as a null terminated string and processed that way. (https://gitlab.com/mbunkus/mkvtoolnix/-/blob/main/src/extract/xtr_textsubs.cpp#L117) My change here simply avoids copying the trailing null terminator(s) present. FATE succeeds as there are no mkvs in the suite that have ASS subtitles embedded. Tim Angus (1): avformat/assenc: fix incorrect copy of null terminator libavformat/assenc.c | 6 ++ 1 file changed, 6 insertions(+) Why is there a gap of submitted patches on patchwork? Have I missed something? https://patchwork.ffmpeg.org/project/ffmpeg/list/ ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/1] avformat/assenc: avoid incorrect copy of null terminator
When writing a subtitle SSA/ASS subtitle file, the
AVCodecParameters::extradata buffer is written directly to the output.
In the case where the buffer is filled from a matroska source file
produced by some older versions of Handbrake, this buffer ends with a
null terminating character, which is then erroneously copied into the
middle of the output file. The refactoring here avoids this problem by
copying the source buffer, manually null terminating it, then treating
it as a string rather than a raw buffer. This way it is agnostic as to
whether the source buffer was null terminated or not.
Signed-off-by: Tim Angus
---
libavformat/assenc.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/libavformat/assenc.c b/libavformat/assenc.c
index 1600f0a02b..4c9ea6f982 100644
--- a/libavformat/assenc.c
+++ b/libavformat/assenc.c
@@ -24,6 +24,7 @@
#include "internal.h"
#include "libavutil/opt.h"
+#include "libavutil/mem.h"
typedef struct DialogueLine {
int readorder;
@@ -55,6 +56,7 @@ static int write_header(AVFormatContext *s)
avpriv_set_pts_info(s->streams[0], 64, 1, 100);
if (par->extradata_size > 0) {
size_t header_size = par->extradata_size;
+char *header_string = NULL;
uint8_t *trailer = strstr(par->extradata, "\n[Events]");
if (trailer)
@@ -69,9 +71,20 @@ static int write_header(AVFormatContext *s)
ass->trailer = trailer;
}
-avio_write(s->pb, par->extradata, header_size);
-if (par->extradata[header_size - 1] != '\n')
-avio_write(s->pb, "\r\n", 2);
+header_string = av_malloc(header_size + 1);
+if (!header_string)
+return AVERROR(ENOMEM);
+
+memcpy(header_string, par->extradata, header_size);
+header_string[header_size] = 0;
+
+avio_printf(s->pb, "%s", header_string);
+
+if (header_string[strlen(header_string) - 1] != '\n')
+avio_printf(s->pb, "\r\n");
+
+av_free(header_string);
+
ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]");
if (!strstr(par->extradata, "\n[Events]"))
avio_printf(s->pb, "[Events]\r\nFormat: %s, Start, End, Style,
Name, MarginL, MarginR, MarginV, Effect, Text\r\n",
--
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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/2] avformat/assenc: fix strstr calls on non-string
In the write_header function of the ASS encoder, extradata is searched for a substring using the strstr function. strstr expects a null terminated C string as its first parameter, but extradata is (notionally) not one of these, meaning that the calls to strstr only happen to work because (presumably) they encounter a 0 byte somewhere downstream of extradata. I say notionally because often extradata will in fact be null terminated (which causes other problems, see parent commit). Signed-off-by: Tim Angus --- libavformat/assenc.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavformat/assenc.c b/libavformat/assenc.c index 4c9ea6f982..f6f5293bb7 100644 --- a/libavformat/assenc.c +++ b/libavformat/assenc.c @@ -83,12 +83,12 @@ static int write_header(AVFormatContext *s) if (header_string[strlen(header_string) - 1] != '\n') avio_printf(s->pb, "\r\n"); -av_free(header_string); - -ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]"); -if (!strstr(par->extradata, "\n[Events]")) +ass->ssa_mode = !strstr(header_string, "\n[V4+ Styles]"); +if (!strstr(header_string, "\n[Events]")) avio_printf(s->pb, "[Events]\r\nFormat: %s, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text\r\n", ass->ssa_mode ? "Marked" : "Layer"); + +av_free(header_string); } return 0; -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avformat/assenc: fix incorrect copy of null terminator
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/2] avformat/assenc: fix strstr calls on non-string
On 31/01/2023 11:13, Tim Angus wrote: In the write_header function of the ASS encoder, extradata is searched for a substring using the strstr function. strstr expects a null terminated C string as its first parameter, but extradata is (notionally) not one of these, meaning that the calls to strstr only happen to work because (presumably) they encounter a 0 byte somewhere downstream of extradata. I say notionally because often extradata will in fact be null terminated (which causes other problems, see parent commit). Signed-off-by: Tim Angus --- libavformat/assenc.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavformat/assenc.c b/libavformat/assenc.c index 4c9ea6f982..f6f5293bb7 100644 --- a/libavformat/assenc.c +++ b/libavformat/assenc.c @@ -83,12 +83,12 @@ static int write_header(AVFormatContext *s) if (header_string[strlen(header_string) - 1] != '\n') avio_printf(s->pb, "\r\n"); -av_free(header_string); - -ass->ssa_mode = !strstr(par->extradata, "\n[V4+ Styles]"); -if (!strstr(par->extradata, "\n[Events]")) +ass->ssa_mode = !strstr(header_string, "\n[V4+ Styles]"); +if (!strstr(header_string, "\n[Events]")) avio_printf(s->pb, "[Events]\r\nFormat: %s, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text\r\n", ass->ssa_mode ? "Marked" : "Layer"); + +av_free(header_string); } return 0; Apparently extradata is padded with zeroes, so this is probably unnecessary, strictly speaking. Having said that, reasoning about why the code works is less obscure with the patch applied. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/1] avformat/assenc: avoid incorrect copy of null terminator
On 27/01/2023 17:20, Tim Angus wrote: When writing a subtitle SSA/ASS subtitle file, the AVCodecParameters::extradata buffer is written directly to the output. In the case where the buffer is filled from a matroska source file produced by some older versions of Handbrake, this buffer ends with a null terminating character, which is then erroneously copied into the middle of the output file. The refactoring here avoids this problem by copying the source buffer, manually null terminating it, then treating it as a string rather than a raw buffer. This way it is agnostic as to whether the source buffer was null terminated or not. Could somebody give this a look please? Thanks. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
