[FFmpeg-devel] [PATCH v2] avformat/dashdec: fix memleak for commit commit e134c203
Steven Liu
lq at chinaffmpeg.org
Fri Mar 20 14:17:24 EET 2020
> 2020年3月20日 下午7:19，Nicolas George <george at nsup.org> 写道：
>
> Steven Liu (12020-03-20):
>> These member will be used for get more correct information of the MPD
>>
>> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> Suggested-by: Nicolas George <george at nsup.org>
>> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
>> ---
>> libavformat/dashdec.c | 180 +++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 151 insertions(+), 29 deletions(-)
>>
>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>> index 5bbe5d3985..df8f30dcb5 100644
>> --- a/libavformat/dashdec.c
>> +++ b/libavformat/dashdec.c
>> @@ -108,6 +108,7 @@ struct representation {
>> int64_t cur_seg_offset;
>> int64_t cur_seg_size;
>> struct fragment *cur_seg;
>> + char *lang;
>>
>> /* Currently active Media Initialization Section */
>> struct fragment *init_section;
>> @@ -122,19 +123,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;
>> + uint64_t adaptionset_minbw;
>> + uint64_t adaptionset_maxbw;
>
>> + uint64_t adaptionset_minwidth;
>> + uint64_t adaptionset_maxwidth;
>> + uint64_t adaptionset_minheight;
>> + uint64_t adaptionset_maxheight;
>
> Unused: remove. Add later if you have something to do with it.
>
>> + AVRational adaptionset_minframerate;
>> + AVRational adaptionset_maxframerate;
>
>> + int adaptionset_segmentalignment;
>> + int adaptionset_bitstreamswitching;
>
> Unused: remove.
>
>>
>> int n_videos;
>> struct representation **videos;
>> @@ -169,6 +168,51 @@ typedef struct DASHContext {
>>
>> } DASHContext;
>>
>
>> +#define DASH_GET_PROP_INT64(node, member, key) \
>
> No reason to make it a macro: make it a function.
>
>> +{ \
>
>> + char *val = NULL; \
>
> Useless initialization.
>
>> + char *end_ptr = NULL; \
>> + val = xmlGetProp((node), (key)); \
>
>> + (member) = 0; \
>
> This overwrites previous initializations, like "c->adaptionset_maxbw =
> INT64_MAX".
>
>> + if (val) { \
>
>> + (member) = strtoull(val, &end_ptr, 10); \
>
> If the value is syntactically incorrect, the error return value of
> strtoull() will be stored in member, this is not good: use a temp
> variable.
>
>> + if (errno == ERANGE) \
>> + av_log((s), AV_LOG_WARNING, "overflow/underflow when get value of %s\n", (key) ); \
>
> And do nothing?
>
>> + if (end_ptr == val || end_ptr[0] != '\0') { \
>> + av_log(s, AV_LOG_ERROR, "The %s field value is " \
>> + "not a valid number: %s\n", (key), val); \
>> + xmlFree(val); \
>
>> + return AVERROR(EINVAL); \
>
> This is not an invalid value from the user, it's from a file:
> INVALIDDATA.
>
> Also, sometimes you treat an invalid value as an error, sometimes as a
> warning: inconsistent.
>
>> + } \
>> + xmlFree(val); \
>> + } \
>> +}
>> +
>
>> +#define DASH_GET_PROP_RATIO(node, member, key) \
>
> No need to make it a macro.
>
>> +{ \
>> + char *val = NULL; \
>> + val = xmlGetProp((node), (key)); \
>
>> + (member) = av_make_q(0, 0); \
>
> Redundant init.
>
>> + if (val) { \
>> + ret = get_ratio_from_string(&(member), val); \
>> + if (ret < 0) \
>> + av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", (key), val); \
>> + xmlFree(val); \
>> + } \
>
> Leak.
Which one leak? if xmlGetProp return null, val should null and dose not leak.
/**
* xmlGetProp:
* @node: the node
* @name: the attribute name
*
* Search and get the value of an attribute associated to a node
* This does the entity substitution.
* This function looks in DTD attribute declaration for #FIXED or
* default declaration values unless DTD use has been turned off.
* NOTE: this function acts independently of namespaces associated
* to the attribute. Use xmlGetNsProp() or xmlGetNoNsProp()
* for namespace aware processing.
*
* Returns the attribute value or NULL if not found.
* It's up to the caller to free the memory with xmlFree().
*/
xmlChar *
xmlGetProp(const xmlNode *node, const xmlChar *name) {
xmlAttrPtr prop;
prop = xmlHasProp(node, name);
if (prop == NULL)
return(NULL);
return(xmlGetPropNodeValueInternal(prop));
}
>
>> +}
>> +
>> +#define DASH_GET_PROP_BOOL(node, member, key) \
>
> No need to make it a macro.
>
>> +{ \
>> + char *val = NULL; \
>> + val = xmlGetProp((node), (key)); \
>> + (member) = 0; \
>
> Redundant init.
>
>> + if (val) { \
>> + if (!av_strcasecmp(val, "true")) \
>> + (member) = 1; \
>> + xmlFree(val); \
>> + } \
>
> Leak.
Ask same as above.
>
>> +}
>> +
>> static int ishttp(char *url)
>> {
>> const char *proto_name = avio_find_protocol_name(url);
>> @@ -406,6 +450,32 @@ static void free_subtitle_list(DASHContext *c)
>> c->n_subtitles = 0;
>> }
>>
>> +#define CHECK_STRTOL_RESULT(keystr, dest) \
>> +{ \
>> + char *end_ptr = NULL; \
>> + if ((keystr)) { \
>> + (dest) = (int) strtol(keystr, &end_ptr, 10); \
>> + if (errno == ERANGE) { \
>> + return AVERROR(ERANGE); \
>> + } \
>> + if (end_ptr == keystr || end_ptr[0] != '\0') { \
>> + return AVERROR(EINVAL); \
>> + } \
>> + } \
>> +}
>> +static int get_ratio_from_string(AVRational *dst, char *src)
>> +{
>> + char *start = NULL;
>> + char *end = NULL;
>> +
>> + dst->den = 1;
>
>> + start = av_strtok(src, "/", &end);
>> + CHECK_STRTOL_RESULT(start, dst->num)
>> + CHECK_STRTOL_RESULT(end, dst->den)
>
> end can be NULL. Don't use av_strtok(), it's not the proper function
> here. Use strtol(), it returns to you the end of the number. It's bad
> style to modify a string while parsing if it's not necessary.
The end can be null, there can only maxFrameRate=25, or maxFrameRate=60/2.
>> +
>> + return 0;
>> +}
>> +
>> static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url,
>> AVDictionary *opts, AVDictionary *opts2, int *is_http)
>> {
>> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>> ret = AVERROR(ENOMEM);
>> goto end;
>> }
>> + if (c->adaptionset_lang) {
>> + rep->lang = av_strdup(c->adaptionset_lang);
>> + if (!rep->lang) {
>> + av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
>> + av_freep(&rep);
>> + ret = AVERROR(ENOMEM);
>> + goto end;
>> + }
>> + }
>> rep->parent = s;
>> representation_segmenttemplate_node = find_child_node_by_name(representation_node, "SegmentTemplate");
>> representation_baseurl_node = find_child_node_by_name(representation_node, "BaseURL");
>> @@ -1073,12 +1152,25 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>> if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>> rep->fragment_timescale = 1;
>> rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
>
>> + if (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)
>> + av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s id incorrect\n", rep_id_val);
>> +
>
> This warning seems to be of dubious use: warnings should be useful to
> users; what can a user do here?
>
>> strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>> rep->framerate = av_make_q(0, 0);
>> if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
>> ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
>> if (ret < 0)
>> - av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>> + av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>
>> + if (c->adaptionset_maxframerate.num > 0) {
>> + if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1)
>> + av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>> + "higher than max framerate, you should check the mpd\n", rep_id_val);
>> + }
>> + if (c->adaptionset_minframerate.num > 0)
>> + if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
>> + av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>> + "lower than min framerate, you should check the mpd\n", rep_id_val);
>> + }
>
> Ditto.
Just tell the user this value is incorrect in mpd, the result maybe does not accord with the expected result.
User should check the mpd file content is correct.
>
>> }
>>
>> switch (type) {
>> @@ -1116,6 +1208,34 @@ end:
>> return ret;
>> }
>>
>> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
>> +{
>> + int ret = 0;
>> + DASHContext *c = s->priv_data;
>> +
>> + if (!adaptionset_node) {
>> + av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
>> + return AVERROR(EINVAL);
>> + }
>> + c->adaptionset_minframerate = av_make_q(0, 0);
>> + c->adaptionset_maxbw = INT64_MAX;
>> + c->adaptionset_minbw = 0;
>> + c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
>> +
>> + DASH_GET_PROP_INT64(adaptionset_node, c->adaptionset_minbw, "minBandwidth")
>> + DASH_GET_PROP_INT64(adaptionset_node, c->adaptionset_maxbw, "maxBandwidth")
>> + DASH_GET_PROP_INT64(adaptionset_node, c->adaptionset_minwidth, "minWidth")
>> + DASH_GET_PROP_INT64(adaptionset_node, c->adaptionset_maxwidth, "maxWidth")
>> + DASH_GET_PROP_INT64(adaptionset_node, c->adaptionset_minheight, "minHeight")
>> + DASH_GET_PROP_INT64(adaptionset_node, c->adaptionset_maxheight, "maxHeight")
>> + DASH_GET_PROP_RATIO(adaptionset_node, c->adaptionset_minframerate, "minFrameRate")
>> + DASH_GET_PROP_RATIO(adaptionset_node, c->adaptionset_maxframerate, "maxFrameRate")
>> + DASH_GET_PROP_BOOL(adaptionset_node, c->adaptionset_segmentalignment, "segmentAlignment")
>> + DASH_GET_PROP_BOOL(adaptionset_node, c->adaptionset_bitstreamswitching, "bitstreamSwitching")
>> +
>> + return ret;
>> +}
>> +
>> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>> xmlNodePtr adaptionset_node,
>> xmlNodePtr mpd_baseurl_node,
>> @@ -1131,19 +1251,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>> 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) {
>> @@ -1170,12 +1281,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>> adaptionset_segmentlist_node,
>> adaptionset_supplementalproperty_node);
>> if (ret < 0) {
>> - return ret;
>> + goto end;
>> }
>> }
>> node = xmlNextElementSibling(node);
>> }
>> - return 0;
>> +
>> +end:
>> + av_freep(&c->adaptionset_lang);
>> + return ret;
>> }
>>
>> static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
>> @@ -2148,6 +2262,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 (rep->lang) {
>> + av_dict_set(&rep->assoc_stream->metadata, "lang", rep->lang, 0);
>> + av_freep(&rep->lang);
>> + }
>> }
>> for (i = 0; i < c->n_subtitles; i++) {
>> rep = c->subtitles[i];
>> @@ -2155,6 +2273,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 (rep->lang) {
>> + av_dict_set(&rep->assoc_stream->metadata, "lang", rep->lang, 0);
>> + av_freep(&rep->lang);
>> + }
>> }
>> }
>>
>
> Regards,
>
> --
> Nicolas George
Thanks
Steven Liu
More information about the ffmpeg-devel
mailing list