avformat/oggparsevorbis: Avoid tmp bufs when parsing VorbisComment

A single VorbisComment consists of a length field and a
non-NUL-terminated string of the form "key=value". Up until now,
when parsing such a VorbisComment, zero-terminated duplicates of
key and value would be created. This is wasteful if these duplicates
are freed shortly afterwards, as happens in particular in case of
attached pictures: In this case value is base64 encoded and only
needed to decode the actual data.

Therefore this commit changes this: The buffer is temporarily modified
so that both key and value are zero-terminated. Then the data is used
in-place and restored to its original state afterwards.

This requires that the buffer has at least one byte of padding. All
buffers currently have AV_INPUT_BUFFER_PADDING_SIZE bytes padding,
so this is ok.

Finally, this also fixes weird behaviour from ogm_chapter():
It sometimes freed given to it, leaving the caller with dangling
pointers.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>

2 files changed, 39 insertions, 33 deletions

diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index 4cce53de41..1d28e50aa8 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -130,9 +130,25 @@ extern const struct ogg_codec ff_theora_codec;
 extern const struct ogg_codec ff_vorbis_codec;
 extern const struct ogg_codec ff_vp8_codec;
 
+/**
+ * Parse Vorbis comments
+ *
+ * @note  The buffer will be temporarily modifed by this function,
+ *        so it needs to be writable. Furthermore it must be padded
+ *        by a single byte (not counted in size).
+ *        All changes will have been reverted upon return.
+ */
 int ff_vorbis_comment(AVFormatContext *ms, AVDictionary **m,
                       const uint8_t *buf, int size, int parse_picture);
 
+/**
+ * Parse Vorbis comments and add metadata to an AVStream
+ *
+ * @note  The buffer will be temporarily modifed by this function,
+ *        so it needs to be writable. Furthermore it must be padded
+ *        by a single byte (not counted in size).
+ *        All changes will have been reverted upon return.
+ */
 int ff_vorbis_stream_comment(AVFormatContext *as, AVStream *st,
                              const uint8_t *buf, int size);
 
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 103a8784ab..b66167ba62 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -39,7 +39,7 @@
 #include "vorbiscomment.h"
 #include "replaygain.h"
 
-static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
+static int ogm_chapter(AVFormatContext *as, const uint8_t *key, const uint8_t *val)
 {
     int i, cnum, h, m, s, ms, keylen = strlen(key);
     AVChapter *chapter = NULL;
@@ -54,7 +54,6 @@ static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
         avpriv_new_chapter(as, cnum, (AVRational) { 1, 1000 },
                            ms + 1000 * (s + 60 * (m + 60 * h)),
                            AV_NOPTS_VALUE, NULL);
-        av_free(val);
     } else if (!av_strcasecmp(key + keylen - 4, "NAME")) {
         for (i = 0; i < as->nb_chapters; i++)
             if (as->chapters[i]->id == cnum) {
@@ -64,11 +63,10 @@ static int ogm_chapter(AVFormatContext *as, uint8_t *key, uint8_t *val)
         if (!chapter)
             return 0;
 
-        av_dict_set(&chapter->metadata, "title", val, AV_DICT_DONT_STRDUP_VAL);
+        av_dict_set(&chapter->metadata, "title", val, 0);
     } else
         return 0;
 
-    av_free(key);
     return 1;
 }
 
@@ -84,13 +82,18 @@ int ff_vorbis_stream_comment(AVFormatContext *as, AVStream *st,
     return updates;
 }
 
+/**
+ * This function temporarily modifies the (const qualified) input buffer
+ * and reverts its changes before return. The input buffer needs to have
+ * at least one byte of padding.
+ */
 static int vorbis_parse_single_comment(AVFormatContext *as, AVDictionary **m,
                                        const uint8_t *buf, uint32_t size,
                                        int *updates, int parse_picture)
 {
-    const char *t = buf, *v = memchr(t, '=', size);
-    char *tt, *ct;
+    char *t = (char*)buf, *v = memchr(t, '=', size);
     int tl, vl;
+    char backup;
 
     if (!v)
         return 0;
@@ -102,19 +105,10 @@ static int vorbis_parse_single_comment(AVFormatContext *as, AVDictionary **m,
     if (!tl || !vl)
         return 0;
 
-    tt = av_malloc(tl + 1);
-    ct = av_malloc(vl + 1);
-    if (!tt || !ct) {
-        av_freep(&tt);
-        av_freep(&ct);
-        return AVERROR(ENOMEM);
-    }
-
-    memcpy(tt, t, tl);
-    tt[tl] = 0;
+    t[tl]  = 0;
 
-    memcpy(ct, v, vl);
-    ct[vl] = 0;
+    backup = v[vl];
+    v[vl]  = 0;
 
     /* The format in which the pictures are stored is the FLAC format.
      * Xiph says: "The binary FLAC picture structure is base64 encoded
@@ -122,35 +116,31 @@ static int vorbis_parse_single_comment(AVFormatContext *as, AVDictionary **m,
      * 'METADATA_BLOCK_PICTURE'. This is the preferred and
      * recommended way of embedding cover art within VorbisComments."
      */
-    if (!av_strcasecmp(tt, "METADATA_BLOCK_PICTURE") && parse_picture) {
+    if (!av_strcasecmp(t, "METADATA_BLOCK_PICTURE") && parse_picture) {
         int ret, len = AV_BASE64_DECODE_SIZE(vl);
         uint8_t *pict = av_malloc(len);
 
         if (!pict) {
             av_log(as, AV_LOG_WARNING, "out-of-memory error. Skipping cover art block.\n");
-            av_freep(&tt);
-            av_freep(&ct);
-            return 0;
+            goto end;
         }
-        ret = av_base64_decode(pict, ct, len);
-        av_freep(&tt);
-        av_freep(&ct);
+        ret = av_base64_decode(pict, v, len);
         if (ret > 0)
             ret = ff_flac_parse_picture(as, pict, ret, 0);
         av_freep(&pict);
         if (ret < 0) {
             av_log(as, AV_LOG_WARNING, "Failed to parse cover art block.\n");
-            return 0;
+            goto end;
         }
-    } else if (!ogm_chapter(as, tt, ct)) {
+    } else if (!ogm_chapter(as, t, v)) {
         (*updates)++;
-        if (av_dict_get(*m, tt, NULL, 0)) {
-            av_dict_set(m, tt, ";", AV_DICT_APPEND);
-        }
-        av_dict_set(m, tt, ct,
-                    AV_DICT_DONT_STRDUP_KEY | AV_DICT_DONT_STRDUP_VAL |
-                    AV_DICT_APPEND);
+        if (av_dict_get(*m, t, NULL, 0))
+            av_dict_set(m, t, ";", AV_DICT_APPEND);
+        av_dict_set(m, t, v, AV_DICT_APPEND);
     }
+end:
+    t[tl] = '=';
+    v[vl] = backup;
 
     return 0;
 }