| Message ID | 20200320024900.8436-1-lq@chinaffmpeg.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v2] avformat/dashdec: fix memleak for commit commit e134c203 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Steven Liu (12020-03-20): > These member will be used for get more correct information of the MPD > > Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > Suggested-by: Nicolas George <george@nsup.org> > Signed-off-by: Steven Liu <lq@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. > +} > + > +#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. > +} > + > 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. > + > + 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. > } > > 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,
> 2020年3月20日 下午7:19，Nicolas George <george@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@gmail.com> >> Suggested-by: Nicolas George <george@nsup.org> >> Signed-off-by: Steven Liu <lq@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
Steven Liu (12020-03-20): > Which one leak? if xmlGetProp return null, val should null and dose not leak. Yes, my mistake. > The end can be null, there can only maxFrameRate=25, or maxFrameRate=60/2. It is a value from the outside, you can't trust it on pail of security exploit. > 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. Which user knows about the mpd? Regards,
> 2020年3月20日 下午8:25，Nicolas George <george@nsup.org> 写道： > >> The end can be null, there can only maxFrameRate=25, or maxFrameRate=60/2. > > It is a value from the outside, you can't trust it on pail of security > exploit. Yes, av_strtok get the first part and the other part, And use the strtol get the first part to long, it check the string range, and get the endpoint of (not number part.) The end is same as the first part. I cannot sure if I misunderstand the usage, I think I need one example of the security exploit. > >> 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. > > Which user knows about the mpd? I think you are right , let me think how to do it. Thanks Steven Liu
Steven Liu (12020-03-20):
> To: Nicolas George <george@nsup.org>
Please heed reply-to headers.
Regards,
Steven Liu (12020-03-20): > Yes, av_strtok get the first part and the other part, > And use the strtol get the first part to long, it check the string range, and get the endpoint of (not number part.) > The end is same as the first part. My exact comment was wrong. The actual problem is that av_strtok() does not tell you the semantic of saveptr, you can therefore only use it as an argument to another call to av_strtok(). Regards,
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; + AVRational adaptionset_minframerate; + AVRational adaptionset_maxframerate; + int adaptionset_segmentalignment; + int adaptionset_bitstreamswitching; int n_videos; struct representation **videos; @@ -169,6 +168,51 @@ typedef struct DASHContext { } DASHContext; +#define DASH_GET_PROP_INT64(node, member, key) \ +{ \ + char *val = NULL; \ + char *end_ptr = NULL; \ + val = xmlGetProp((node), (key)); \ + (member) = 0; \ + if (val) { \ + (member) = strtoull(val, &end_ptr, 10); \ + if (errno == ERANGE) \ + av_log((s), AV_LOG_WARNING, "overflow/underflow when get value of %s\n", (key) ); \ + 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); \ + } \ + xmlFree(val); \ + } \ +} + +#define DASH_GET_PROP_RATIO(node, member, key) \ +{ \ + char *val = NULL; \ + val = xmlGetProp((node), (key)); \ + (member) = av_make_q(0, 0); \ + 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); \ + } \ +} + +#define DASH_GET_PROP_BOOL(node, member, key) \ +{ \ + char *val = NULL; \ + val = xmlGetProp((node), (key)); \ + (member) = 0; \ + if (val) { \ + if (!av_strcasecmp(val, "true")) \ + (member) = 1; \ + xmlFree(val); \ + } \ +} + 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) + + 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); + 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); + } } 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); + } } }
These member will be used for get more correct information of the MPD Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> Suggested-by: Nicolas George <george@nsup.org> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/dashdec.c | 180 +++++++++++++++++++++++++++++++++++------- 1 file changed, 151 insertions(+), 29 deletions(-)