[FFmpeg-devel] [PATCH] libaformat: fix incorrect handling of incomplete AVBPrint.

* [FFmpeg-devel] [PATCH] libaformat: fix incorrect handling of incomplete AVBPrint.
@ 2023-06-22 21:04 Reimar.Doeffinger
  2023-06-23  1:01 ` James Almer
  0 siblings, 1 reply; 8+ messages in thread
From: Reimar.Doeffinger @ 2023-06-22 21:04 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Reimar Döffinger

From: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

Change some internal APIs a bit to make it harder to make
such mistakes.
In particular, have the read chunk functions return an error
when the result is incomplete.
This might be less flexible, but since there has been no
use-case for that so far, avoiding coding mistakes seems better.
Add a function to queue a AVBPrint directly (ff_subtitles_queue_insert_bprint).
Also fixes a leak in lrcdec when ff_subtitles_queue_insert fails.
---
Note that this combines a few different things, but they all
are meant to address the same issue.
Happy to split if that's wanted, first priority is getting an
idea if some part of this seems like a bad idea generally.

 libavformat/assdec.c         |  4 +++-
 libavformat/lrcdec.c         |  7 ++++++-
 libavformat/mpsubdec.c       |  5 +++--
 libavformat/realtextdec.c    |  7 ++++++-
 libavformat/samidec.c        |  7 ++++++-
 libavformat/srtdec.c         |  4 +++-
 libavformat/subtitles.c      | 17 +++++++++++++----
 libavformat/subtitles.h      | 14 ++++++++++++--
 libavformat/tedcaptionsdec.c |  2 +-
 libavformat/webvttdec.c      |  4 +++-
 10 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/libavformat/assdec.c b/libavformat/assdec.c
index 0915f6fafd..bf7b8a73a2 100644
--- a/libavformat/assdec.c
+++ b/libavformat/assdec.c
@@ -73,6 +73,8 @@ static int read_dialogue(ASSContext *ass, AVBPrint *dst, const uint8_t *p,
 
         av_bprint_clear(dst);
         av_bprintf(dst, "%u,%d,%s", ass->readorder++, layer, p + pos);
+        if (!av_bprint_is_complete(dst))
+            return AVERROR(ENOMEM);
 
         /* right strip the buffer */
         while (dst->len > 0 &&
@@ -135,7 +137,7 @@ static int ass_read_header(AVFormatContext *s)
             av_bprintf(&header, "%s", line.str);
             continue;
         }
-        sub = ff_subtitles_queue_insert(&ass->q, rline.str, rline.len, 0);
+        sub = ff_subtitles_queue_insert_bprint(&ass->q, &rline, 0);
         if (!sub) {
             res = AVERROR(ENOMEM);
             goto end;
diff --git a/libavformat/lrcdec.c b/libavformat/lrcdec.c
index fff39495f8..83bb4a4b75 100644
--- a/libavformat/lrcdec.c
+++ b/libavformat/lrcdec.c
@@ -171,6 +171,8 @@ static int lrc_read_header(AVFormatContext *s)
 
     while(!avio_feof(s->pb)) {
         int64_t pos = read_line(&line, s->pb);
+        if (!av_bprint_is_complete(&line))
+            goto err_nomem_out;
         int64_t header_offset = find_header(line.str);
         if(header_offset >= 0) {
             char *comma_offset = strchr(line.str, ':');
@@ -205,7 +207,7 @@ static int lrc_read_header(AVFormatContext *s)
                 sub = ff_subtitles_queue_insert(&lrc->q, line.str + ts_strlength,
                                                 line.len - ts_strlength, 0);
                 if (!sub)
-                    return AVERROR(ENOMEM);
+                    goto err_nomem_out;
                 sub->pos = pos;
                 sub->pts = ts_start - lrc->ts_offset;
                 sub->duration = -1;
@@ -216,6 +218,9 @@ static int lrc_read_header(AVFormatContext *s)
     ff_metadata_conv_ctx(s, NULL, ff_lrc_metadata_conv);
     av_bprint_finalize(&line, NULL);
     return 0;
+err_nomem_out:
+    av_bprint_finalize(&line, NULL);
+    return AVERROR(ENOMEM);
 }
 
 const AVInputFormat ff_lrc_demuxer = {
diff --git a/libavformat/mpsubdec.c b/libavformat/mpsubdec.c
index d290a41fb9..0374563575 100644
--- a/libavformat/mpsubdec.c
+++ b/libavformat/mpsubdec.c
@@ -116,9 +116,10 @@ static int mpsub_read_header(AVFormatContext *s)
             AVPacket *sub;
             const int64_t pos = avio_tell(s->pb);
 
-            ff_subtitles_read_chunk(s->pb, &buf);
+            res = ff_subtitles_read_chunk(s->pb, &buf);
+            if (res < 0) goto end;
             if (buf.len) {
-                sub = ff_subtitles_queue_insert(&mpsub->q, buf.str, buf.len, 0);
+                sub = ff_subtitles_queue_insert_bprint(&mpsub->q, &buf, 0);
                 if (!sub) {
                     res = AVERROR(ENOMEM);
                     goto end;
diff --git a/libavformat/realtextdec.c b/libavformat/realtextdec.c
index c281dec346..9f6aab789e 100644
--- a/libavformat/realtextdec.c
+++ b/libavformat/realtextdec.c
@@ -80,6 +80,11 @@ static int realtext_read_header(AVFormatContext *s)
         const int64_t pos = ff_text_pos(&tr) - (c != 0);
         int n = ff_smil_extract_next_text_chunk(&tr, &buf, &c);
 
+        if (n < 0)
+        {
+            res = n;
+            goto end;
+        }
         if (n == 0)
             break;
 
@@ -103,7 +108,7 @@ static int realtext_read_header(AVFormatContext *s)
             /* if we just read a <time> tag, introduce a new event, otherwise merge
              * with the previous one */
             int merge = !av_strncasecmp(buf.str, "<time", 5) ? 0 : 1;
-            sub = ff_subtitles_queue_insert(&rt->q, buf.str, buf.len, merge);
+            sub = ff_subtitles_queue_insert_bprint(&rt->q, &buf, merge);
             if (!sub) {
                 res = AVERROR(ENOMEM);
                 goto end;
diff --git a/libavformat/samidec.c b/libavformat/samidec.c
index 0da299343d..6fff6019bf 100644
--- a/libavformat/samidec.c
+++ b/libavformat/samidec.c
@@ -68,6 +68,11 @@ static int sami_read_header(AVFormatContext *s)
         const int64_t pos = ff_text_pos(&tr) - (c != 0);
         int is_sync, is_body, n = ff_smil_extract_next_text_chunk(&tr, &buf, &c);
 
+        if (n < 0)
+        {
+            res = n;
+            goto end;
+        }
         if (n == 0)
             break;
 
@@ -84,7 +89,7 @@ static int sami_read_header(AVFormatContext *s)
         if (!got_first_sync_point) {
             av_bprintf(&hdr_buf, "%s", buf.str);
         } else {
-            sub = ff_subtitles_queue_insert(&sami->q, buf.str, buf.len, !is_sync);
+            sub = ff_subtitles_queue_insert_bprint(&sami->q, &buf, !is_sync);
             if (!sub) {
                 res = AVERROR(ENOMEM);
                 av_bprint_finalize(&hdr_buf, NULL);
diff --git a/libavformat/srtdec.c b/libavformat/srtdec.c
index bf02450555..635c1550b9 100644
--- a/libavformat/srtdec.c
+++ b/libavformat/srtdec.c
@@ -97,12 +97,14 @@ static int add_event(FFDemuxSubtitlesQueue *q, AVBPrint *buf, char *line_cache,
     if (append_cache && line_cache[0])
         av_bprintf(buf, "%s\n", line_cache);
     line_cache[0] = 0;
+    if (!av_bprint_is_complete(buf))
+        return AVERROR(ENOMEM);
 
     while (buf->len > 0 && buf->str[buf->len - 1] == '\n')
         buf->str[--buf->len] = 0;
 
     if (buf->len) {
-        AVPacket *sub = ff_subtitles_queue_insert(q, buf->str, buf->len, 0);
+        AVPacket *sub = ff_subtitles_queue_insert_bprint(q, buf, 0);
         if (!sub)
             return AVERROR(ENOMEM);
         av_bprint_clear(buf);
diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
index 3ba5e2b217..96945a75dd 100644
--- a/libavformat/subtitles.c
+++ b/libavformat/subtitles.c
@@ -145,6 +145,13 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q,
     return sub;
 }
 
+AVPacket *ff_subtitles_queue_insert_bprint(FFDemuxSubtitlesQueue *q,
+                                           const AVBPrint *event, int merge)
+{
+    if (!av_bprint_is_complete(event)) return NULL;
+    return ff_subtitles_queue_insert(q, event->str, event->len, merge);
+}
+
 static int cmp_pkt_sub_ts_pos(const void *a, const void *b)
 {
     const AVPacket *s1 = *(const AVPacket **)a;
@@ -347,13 +354,14 @@ int ff_smil_extract_next_text_chunk(FFTextReader *tr, AVBPrint *buf, char *c)
     do {
         av_bprint_chars(buf, *c, 1);
         *c = ff_text_r8(tr);
+        if (i == INT_MAX) return AVERROR_INVALIDDATA;
         i++;
     } while (*c != end_chr && *c);
     if (end_chr == '>') {
         av_bprint_chars(buf, '>', 1);
         *c = 0;
     }
-    return i;
+    return av_bprint_is_complete(buf) ? i : AVERROR(ENOMEM);
 }
 
 const char *ff_smil_get_attr_ptr(const char *s, const char *attr)
@@ -381,7 +389,7 @@ static inline int is_eol(char c)
     return c == '\r' || c == '\n';
 }
 
-void ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf)
+int ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf)
 {
     char eol_buf[5], last_was_cr = 0;
     int n = 0, i = 0, nb_eol = 0;
@@ -421,15 +429,16 @@ void ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf)
         av_bprint_chars(buf, c, 1);
         n++;
     }
+    return av_bprint_is_complete(buf) ? 0 : AVERROR(ENOMEM);
 }
 
-void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf)
+int ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf)
 {
     FFTextReader tr;
     tr.buf_pos = tr.buf_len = 0;
     tr.type = 0;
     tr.pb = pb;
-    ff_subtitles_read_text_chunk(&tr, buf);
+    return ff_subtitles_read_text_chunk(&tr, buf);
 }
 
 ptrdiff_t ff_subtitles_read_line(FFTextReader *tr, char *buf, size_t size)
diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h
index 4460efacf3..f6688b71d7 100644
--- a/libavformat/subtitles.h
+++ b/libavformat/subtitles.h
@@ -120,6 +120,12 @@ typedef struct {
 AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q,
                                     const uint8_t *event, size_t len, int merge);
 
+/**
+ * Same as ff_subtitles_queue_insert but takes an AVBPrint input.
+ * Avoids common errors like handling incomplete AVBPrint.
+ */
+AVPacket *ff_subtitles_queue_insert_bprint(FFDemuxSubtitlesQueue *q,
+                                           const AVBPrint *event, int merge);
 /**
  * Set missing durations, sort subtitles by PTS (and then byte position), and
  * drop duplicated events.
@@ -155,6 +161,7 @@ int ff_subtitles_read_close(AVFormatContext *s);
  * SMIL helper to load next chunk ("<...>" or untagged content) in buf.
  *
  * @param c cached character, to avoid a backward seek
+ * @return size of chunk or error, e.g. AVERROR(ENOMEM) on incomplete buf
  */
 int ff_smil_extract_next_text_chunk(FFTextReader *tr, AVBPrint *buf, char *c);
 
@@ -169,7 +176,8 @@ const char *ff_smil_get_attr_ptr(const char *s, const char *attr);
 /**
  * @brief Same as ff_subtitles_read_text_chunk(), but read from an AVIOContext.
  */
-void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
+av_warn_unused_result
+int ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
 
 /**
  * @brief Read a subtitles chunk from FFTextReader.
@@ -181,10 +189,12 @@ void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
  *
  * @param tr  I/O context
  * @param buf an initialized buf where the chunk is written
+ * @return 0 on success, error value otherwise, e.g. AVERROR(ENOMEM) if buf is incomplete
  *
  * @note buf is cleared before writing into it.
  */
-void ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf);
+av_warn_unused_result
+int ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf);
 
 /**
  * Get the number of characters to increment to jump to the next line, or to
diff --git a/libavformat/tedcaptionsdec.c b/libavformat/tedcaptionsdec.c
index 31197349b0..a8aa9d9cf9 100644
--- a/libavformat/tedcaptionsdec.c
+++ b/libavformat/tedcaptionsdec.c
@@ -244,7 +244,7 @@ static int parse_file(AVIOContext *pb, FFDemuxSubtitlesQueue *subs)
             ret = AVERROR_INVALIDDATA;
             goto fail;
         }
-        pkt = ff_subtitles_queue_insert(subs, content.str, content.len, 0);
+        pkt = ff_subtitles_queue_insert_bprint(subs, &content, 0);
         if (!pkt) {
             ret = AVERROR(ENOMEM);
             goto fail;
diff --git a/libavformat/webvttdec.c b/libavformat/webvttdec.c
index 52320ba7d1..0b2fc77168 100644
--- a/libavformat/webvttdec.c
+++ b/libavformat/webvttdec.c
@@ -81,7 +81,9 @@ static int webvtt_read_header(AVFormatContext *s)
         size_t identifier_len, settings_len;
         int64_t ts_start, ts_end;
 
-        ff_subtitles_read_chunk(s->pb, &cue);
+        res = ff_subtitles_read_chunk(s->pb, &cue);
+        if (res < 0)
+            goto end;
 
         if (!cue.len)
             break;
-- 
2.37.1 (Apple Git-137.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".

^ permalink raw reply	[flat|nested] 8+ messages in thread