[FFmpeg-devel] [PATCH] avformat/dashdec: fix memleak for commit commit e134c203
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Wed Mar 18 18:48:10 EET 2020
Hello,
before the actual review I want to tell you that I actually agree with
Nicholas. I don't see the point of already parsing stuff that is not
used yet, especially if it involves allocations and checks.
Steven Liu:
> These member will be used for get more correct information of the MPD
>
> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
> ---
> libavformat/dashdec.c | 134 +++++++++++++++++++++++++++++++++---------
> 1 file changed, 107 insertions(+), 27 deletions(-)
>
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index 5bbe5d3985..27d44c2633 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -122,19 +122,17 @@ struct representation {
> typedef struct DASHContext {
> const AVClass *class;
> char *base_url;
> - char *adaptionset_contenttype_val;
> - char *adaptionset_par_val;
> - char *adaptionset_lang_val;
> - char *adaptionset_minbw_val;
> - char *adaptionset_maxbw_val;
> - char *adaptionset_minwidth_val;
> - char *adaptionset_maxwidth_val;
> - char *adaptionset_minheight_val;
> - char *adaptionset_maxheight_val;
> - char *adaptionset_minframerate_val;
> - char *adaptionset_maxframerate_val;
> - char *adaptionset_segmentalignment_val;
> - char *adaptionset_bitstreamswitching_val;
> + char *adaptionset_lang;
> + int64_t adaptionset_minbw;
> + int64_t adaptionset_maxbw;
> + int64_t adaptionset_minwidth;
> + int64_t adaptionset_maxwidth;
> + int64_t adaptionset_minheight;
> + int64_t adaptionset_maxheight;
> + AVRational adaptionset_minframerate;
> + AVRational adaptionset_maxframerate;
> + int adaptionset_segmentalignment;
> + int adaptionset_bitstreamswitching;
>
> int n_videos;
> struct representation **videos;
> @@ -1116,6 +1114,90 @@ end:
> return ret;
> }
>
> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
> +{
> + int ret = 0;
> + DASHContext *c = s->priv_data;
> + char *adaptionset_minbw_val = NULL;
> + char *adaptionset_maxbw_val = NULL;
> + char *adaptionset_minwidth_val = NULL;
> + char *adaptionset_maxwidth_val = NULL;
> + char *adaptionset_minheight_val = NULL;
> + char *adaptionset_maxheight_val = NULL;
> + char *adaptionset_minframerate_val = NULL;
> + char *adaptionset_maxframerate_val = NULL;
> + char *adaptionset_segmentalignment_val = NULL;
> + char *adaptionset_bitstreamswitching_val = NULL;
How about using one pointer for all this stuff? It would still be clear
what is currently parsed because the right-hand side (e.g.
"xmlGetProp(adaptionset_node, "minHeight");") contains it; furthermore,
it allows to make overlong lines shorter.
> +
> + if (!adaptionset_node) {
> + av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
> + return AVERROR(EINVAL);
> + }
> +
> + c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
Is it certain that parse_manifest_adaptionset() (and therefore
parse_manifest_adaptionset_attr() as well) gets only executed once (even
for spec-incompliant input) although it is called inside a loop in
parse_manifest()? If it gets executed more than once, the string from
earlier runs would be overwritten above and therefore leak.
And is there actually a way to distinguish the "node doesn't exist" case
from allocation failure?
> +
> + adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
> + if (adaptionset_minbw_val) {
> + c->adaptionset_minbw = (int64_t) strtoll(adaptionset_minbw_val, NULL, 10);
You don't check for errors here: Check errno as well as the endposition.
Furthermore, is there a reason you are using a signed type for what
appears to be a fundamentally unsigned type.
(How weird that libxml2 doesn't seem to provide a way to parse numbers
without an allocation.)
> + xmlFree(adaptionset_minbw_val);
> + }
> + adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
> + if (adaptionset_maxbw_val) {
> + c->adaptionset_maxbw = (int64_t) strtoll(adaptionset_maxbw_val, NULL, 10);
> + xmlFree(adaptionset_maxbw_val);
> + }
> + adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
> + if (adaptionset_minwidth_val) {
> + c->adaptionset_minwidth = (int64_t) strtoll(adaptionset_minwidth_val, NULL, 10);
> + xmlFree(adaptionset_minwidth_val);
> + }
> + adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
> + if (adaptionset_maxwidth_val) {
> + c->adaptionset_maxwidth = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10);
> + xmlFree(adaptionset_maxwidth_val);
> + }
> + adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
> + if (adaptionset_minheight_val) {
> + c->adaptionset_minheight = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10);
> + xmlFree(adaptionset_minheight_val);
> + }
> + adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
> + if (adaptionset_maxheight_val) {
> + c->adaptionset_maxheight = (int64_t) strtoll(adaptionset_maxheight_val, NULL, 10);
> + xmlFree(adaptionset_maxheight_val);
> + }
> + adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
> + if (adaptionset_minframerate_val) {
> + ret = av_parse_video_rate(&c->adaptionset_minframerate, adaptionset_minframerate_val);
> + xmlFree(adaptionset_minframerate_val);
> + if (ret < 0)
> + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid min frame rate '%s'\n", adaptionset_minframerate_val);
AV_LOG_VERBOSE is clearly wrong. If you ignore the error, it should be
AV_LOG_WARNING; if not AV_LOG_ERROR. You are doing something between
these two: It might be that ret gets overwritten or it might be
returned. If it gets returned, you are skipping
parse_manifest_representation() by simply returning the error to
parse_manifest() which then ignores it.
> + }
> + adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
> + if (adaptionset_maxframerate_val) {
> + ret = av_parse_video_rate(&c->adaptionset_maxframerate, adaptionset_maxframerate_val);
> + xmlFree(adaptionset_maxframerate_val);
> + if (ret < 0)
> + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid max frame rate '%s'\n", adaptionset_maxframerate_val);
> + }
> + adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
> + if (adaptionset_segmentalignment_val) {
> + c->adaptionset_segmentalignment = 0;
> + if (!av_strcasecmp(adaptionset_segmentalignment_val, "true"))
> + c->adaptionset_segmentalignment = 1;
> + xmlFree(adaptionset_segmentalignment_val);
> + }
> + adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
> + if (adaptionset_bitstreamswitching_val) {
> + c->adaptionset_bitstreamswitching = 0;
> + if (!av_strcasecmp(adaptionset_bitstreamswitching_val, "true"))
> + c->adaptionset_bitstreamswitching = 1;
> + xmlFree(adaptionset_bitstreamswitching_val);
> + }
> +
> + return ret;
> +}
> +
> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
> xmlNodePtr adaptionset_node,
> xmlNodePtr mpd_baseurl_node,
> @@ -1124,26 +1206,16 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
> xmlNodePtr period_segmentlist_node)
> {
> int ret = 0;
> - DASHContext *c = s->priv_data;
> xmlNodePtr fragment_template_node = NULL;
> xmlNodePtr content_component_node = NULL;
> xmlNodePtr adaptionset_baseurl_node = NULL;
> xmlNodePtr adaptionset_segmentlist_node = NULL;
> xmlNodePtr adaptionset_supplementalproperty_node = NULL;
> xmlNodePtr node = NULL;
> - c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
> - c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
> - c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
> - c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
> - c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
> - c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
> - c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
> - c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
> - c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
> - c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
> - c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
> - c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
> - c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
> +
> + ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
> + if (ret < 0)
> + return ret;
>
> node = xmlFirstElementChild(adaptionset_node);
> while (node) {
> @@ -2148,6 +2220,10 @@ static int dash_read_header(AVFormatContext *s)
> av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0);
> if (rep->id[0])
> av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0);
> + if (c->adaptionset_lang) {
> + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0);
> + xmlFree(c->adaptionset_lang);
This is very dangerous: You are freeing c->adaptionset_lang, but you are
not resetting the pointer to NULL. This leads to use-after-frees.
Furthermore, I haven't read the relevant documents, but I can't believe
that there can't be multiple different languages for audio and subtitles
(that is after all the main purpose of having different audio and
subtitle streams in the first place!). It would make more sense to have
one language per representation as this seems to correspond to the
streams in FFmpeg.
Additionally, you are ony freeing c->adaptionset_lang if there is an
audio or subtitle stream; a video-only stream would still leak if it had
a defined language. And it would also leak if an error happens so that
we do not reach the lines above (e.g. if av_new_program() fails).
> + }
> }
> for (i = 0; i < c->n_subtitles; i++) {
> rep = c->subtitles[i];
> @@ -2155,6 +2231,10 @@ static int dash_read_header(AVFormatContext *s)
> rep->assoc_stream = s->streams[rep->stream_index];
> if (rep->id[0])
> av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0);
> + if (c->adaptionset_lang) {
> + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0);
> + xmlFree(c->adaptionset_lang);
> + }
> }
> }
>
>
A final note: libxml2 allows to use custom allocator, free and strdup
functions. How about using our functions for this so that we don't need
to worry about e.g. xmlFree(NULL). And we could use av_freep().
- Andreas
PS: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/257908.html
More information about the ffmpeg-devel
mailing list