[FFmpeg-devel] [PATCH] avformat/dashdec: fix memleak for commit commit e134c203
Steven Liu
lq at chinaffmpeg.org
Thu Mar 19 01:30:52 EET 2020
Thanks
Steven Liu
> 2020年3月19日 上午12:48，Andreas Rheinhardt <andreas.rheinhardt at gmail.com> 写道：
>
> 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.
Yes, I agreed with all of you, but I think we should try to "full support all the attributes parse" what writes in the sepcification,
And try to use all the attribute. Because those attribute in the specification is not useless and have some reason, I’m not member of the specification author,
>
> 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.
This will update in version 2.
>
>> +
>> + 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?
This will update in version 2, add the lang into representation->lang for every stream.
>
>> +
>> + 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.
Will update in version
>
> (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;
Ignore the waning shortly, just like the flvenc “duration and file size update failed” at the end of live streaming.
At this should update if the specification said overwritten the framerate, but I don’t see that now, maybe I misunderstand that, need check that more time.
> 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.
Yes, will update in version 2.
>
> 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().
Ok
Thanks
Steven
More information about the ffmpeg-devel
mailing list