diff mbox series

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

Message ID 20230723120016.92184-1-Reimar.Doeffinger@gmx.de
State New
Headers show
Series [FFmpeg-devel] libaformat: fix incorrect handling of incomplete AVBPrint. | expand

Checks

Context Check Description
yinshiyou/make_fate_loongarch64 success Make fate finished
yinshiyou/make_loongarch64 warning New warnings during build
andriy/make_fate_x86 success Make fate finished
andriy/make_x86 warning New warnings during build

Commit Message

Reimar Döffinger July 23, 2023, noon UTC 
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.
---
 libavformat/assdec.c         |  4 +++-
 libavformat/lrcdec.c         |  7 ++++++-
 libavformat/mpsubdec.c       |  5 +++--
 libavformat/realtextdec.c    |  6 +++++-
 libavformat/samidec.c        |  6 +++++-
 libavformat/srtdec.c         |  4 +++-
 libavformat/subtitles.c      | 17 +++++++++++++----
 libavformat/subtitles.h      | 14 ++++++++++++--
 libavformat/tedcaptionsdec.c |  2 +-
 libavformat/webvttdec.c      |  4 +++-
 10 files changed, 54 insertions(+), 15 deletions(-)

Comments

Reimar Döffinger July 27, 2023, 5:26 p.m. UTC | #1 
> On 23 Jul 2023, at 14:00, reimar.doeffinger@gmx.de wrote:
> 
> 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.

Please make any objections or need for more time to review known soon.
Otherwise I plan on pushing in a few days.

Best regards,
Reimar
Nicolas George July 27, 2023, 5:33 p.m. UTC | #2 
Reimar.Doeffinger@gmx.de (12023-07-23):
> 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.
> ---
>  libavformat/assdec.c         |  4 +++-
>  libavformat/lrcdec.c         |  7 ++++++-
>  libavformat/mpsubdec.c       |  5 +++--
>  libavformat/realtextdec.c    |  6 +++++-
>  libavformat/samidec.c        |  6 +++++-
>  libavformat/srtdec.c         |  4 +++-
>  libavformat/subtitles.c      | 17 +++++++++++++----
>  libavformat/subtitles.h      | 14 ++++++++++++--
>  libavformat/tedcaptionsdec.c |  2 +-
>  libavformat/webvttdec.c      |  4 +++-
>  10 files changed, 54 insertions(+), 15 deletions(-)

Sorry I forgot I meant to review. These changes look for the best,
except 

> +    if (!av_bprint_is_complete(event)) return NULL;

> +        if (i == INT_MAX) return AVERROR_INVALIDDATA;

… which do not follow the coding style.

(The dynamic buffer writer of the AVWriter API makes it much harder to
forget checking. But AVWriter is currently in limbo waiting for this
project to reaffirm its orientation.)

Regards,
Reimar Döffinger July 27, 2023, 5:42 p.m. UTC | #3 
> On 27 Jul 2023, at 19:33, Nicolas George <george@nsup.org> wrote:
> 
> Reimar.Doeffinger@gmx.de (12023-07-23):
>> 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.
>> ---
>> libavformat/assdec.c         |  4 +++-
>> libavformat/lrcdec.c         |  7 ++++++-
>> libavformat/mpsubdec.c       |  5 +++--
>> libavformat/realtextdec.c    |  6 +++++-
>> libavformat/samidec.c        |  6 +++++-
>> libavformat/srtdec.c         |  4 +++-
>> libavformat/subtitles.c      | 17 +++++++++++++----
>> libavformat/subtitles.h      | 14 ++++++++++++--
>> libavformat/tedcaptionsdec.c |  2 +-
>> libavformat/webvttdec.c      |  4 +++-
>> 10 files changed, 54 insertions(+), 15 deletions(-)
> 
> Sorry I forgot I meant to review. These changes look for the best,
> except 
> 
>> +    if (!av_bprint_is_complete(event)) return NULL;
> 
>> +        if (i == INT_MAX) return AVERROR_INVALIDDATA;
> 
> … which do not follow the coding style.

Thanks, sent a new version with that updated, plus a fix for a typo
in the commit message.
Nicolas George July 27, 2023, 5:44 p.m. UTC | #4 
Reimar Döffinger (12023-07-27):
> Thanks, sent a new version with that updated, plus a fix for a typo
> in the commit message.

If it is all you have changed, then I do not think I need to look at it
again.

I do not maintain most of the files you have changed, but I think you
can go ahead.

Regards,
Reimar Döffinger July 29, 2023, 2:31 p.m. UTC | #5 
> On 27 Jul 2023, at 19:44, Nicolas George <george@nsup.org> wrote:
> 
> Reimar Döffinger (12023-07-27):
>> Thanks, sent a new version with that updated, plus a fix for a typo
>> in the commit message.
> 
> If it is all you have changed, then I do not think I need to look at it
> again.
> 
> I do not maintain most of the files you have changed, but I think you
> can go ahead.

Thanks, pushed it.
diff mbox series

Patch

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..7992a5b7fc 100644
--- a/libavformat/realtextdec.c
+++ b/libavformat/realtextdec.c
@@ -80,6 +80,10 @@  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 +107,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..070b623ebf 100644
--- a/libavformat/samidec.c
+++ b/libavformat/samidec.c
@@ -68,6 +68,10 @@  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 +88,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;