[FFmpeg-devel] [PATCH 5/8] avformat/oggparsevorbis: Avoid tmp bufs when parsing VorbisComment
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Mon Aug 23 16:16:51 EEST 2021
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 at outlook.com>
---
libavformat/oggdec.h | 16 +++++++++++
libavformat/oggparsevorbis.c | 56 +++++++++++++++---------------------
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 69c15c3dce..463fb61672 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;
}
--
2.30.2
More information about the ffmpeg-devel
mailing list