| Message ID | 20200313191416.13974-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avformat/dashdec: Don't allocate and leak strings that are never used | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Andreas Rheinhardt (2020-03-13 20:14:16) > Since commit e134c203 strdups of several elements of a manifest are kept > in the DASHContext; but said commit completely forgot to free these > strings again (with xmlFree()). Given that these strings are never used > at all, this commit closes this leak by reverting said commit. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/dashdec.c | 27 --------------------------- > 1 file changed, 27 deletions(-) Looks ok.
> 2020年3月14日 上午3:14，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Since commit e134c203 strdups of several elements of a manifest are kept > in the DASHContext; but said commit completely forgot to free these > strings again (with xmlFree()). Given that these strings are never used > at all, this commit closes this leak by reverting said commit. No this is used for fix some problem about full specification support, you can fix the memleak but I don’t think remove them is a good way. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/dashdec.c | 27 --------------------------- > 1 file changed, 27 deletions(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 5bbe5d3985..271202b0a5 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -122,19 +122,6 @@ 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; > > int n_videos; > struct representation **videos; > @@ -1124,26 +1111,12 @@ 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"); > > node = xmlFirstElementChild(adaptionset_node); > while (node) { > -- > 2.20.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Steven Liu (12020-03-16): > No this is used for fix some problem about full specification support, > you can fix the memleak but I don’t think remove them is a good way. How can they fix a problem if they are unused? Regards,
> 2020年3月16日 下午9:19，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-16): >> No this is used for fix some problem about full specification support, >> you can fix the memleak but I don’t think remove them is a good way. > > How can they fix a problem if they are unused? As Andreas said, just xmlFree them at the end of the parse method is ok, I think i will use them for full support do that if you guys don’t know how to do that. > > Regards, > > -- > Nicolas George
Steven Liu (12020-03-16): > > How can they fix a problem if they are unused? > As Andreas said, just xmlFree them at the end of the parse method is ok, > I think i will use them for full support do that if you guys don’t know how to do that. This commit e134c203 was clearly not fully baked. Let's revert it. And if you can contribute a commit that actually does something, with proper lifetime for the variables, we can bring it back. Regards,
> 2020年3月16日 下午9:26，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-16): >>> How can they fix a problem if they are unused? >> As Andreas said, just xmlFree them at the end of the parse method is ok, >> I think i will use them for full support do that if you guys don’t know how to do that. > > This commit e134c203 was clearly not fully baked. Let's revert it. And > if you can contribute a commit that actually does something, with proper > lifetime for the variables, we can bring it back. Let me do it now, don’t remove the code, ok? Keep the code now or add xmlFree them. > > Regards, > > -- > Nicolas George
Steven Liu (12020-03-16): > Let me do it now, don’t remove the code, ok? > Keep the code now or add xmlFree them. There is a leak right now, and adding the xmlFree() is more work than reverting. Also, most of these attributes do not belong as string in the context, they belong in local variable to get parsed immediately. I suggest you make a helper function to parse integer attributes. Reverting is the way to go. Regards,
> 2020年3月16日 下午9:32，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-16): >> Let me do it now, don’t remove the code, ok? >> Keep the code now or add xmlFree them. > > There is a leak right now, and adding the xmlFree() is more work than > reverting. > > Also, most of these attributes do not belong as string in the context, > they belong in local variable to get parsed immediately. I suggest you > make a helper function to parse integer attributes. > > Reverting is the way to go. > I think you can rewrite a dashdec if you have interested in it, there have lots of bug in it, ok? I don’t think just remove some code for it is a good idea, if you want to do some thing for it, you can make it complete, not just remove some code. > Regards, > > -- > Nicolas George
Steven Liu (12020-03-16): > I think you can rewrite a dashdec if you have interested in it, there > have lots of bug in it, ok? > I don’t think just remove some code for it is a good idea, if you want > to do some thing for it, you can make it complete, not just remove > some code. This code is bad: it stores values that are not needed. It should have never gone in in the first place. Write code that works, and it will be reviewed. Feel free to ping me about it. Regards,
> 2020年3月16日 下午9:42，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-16): >> I think you can rewrite a dashdec if you have interested in it, there >> have lots of bug in it, ok? >> I don’t think just remove some code for it is a good idea, if you want >> to do some thing for it, you can make it complete, not just remove >> some code. > > This code is bad: it stores values that are not needed. It should have > never gone in in the first place. Patch welcome for rewrite dashdec. > > Write code that works, and it will be reviewed. Feel free to ping me > about it. > > Regards, > > -- > Nicolas George > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Steven Liu (12020-03-16):
> Patch welcome for rewrite dashdec.
Irrelevant. This particular commit is bad in and by itself. That's what
happens when you push without review.
Regards,
> 2020年3月16日 下午9:48，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-16): >> Patch welcome for rewrite dashdec. > > Irrelevant. This particular commit is bad in and by itself. That's what > happens when you push without review. That is not without review, that is all of you have no interested in it, and I’m the maintainer of it. So I know what I want to do. Thanks Steven
Steven Liu (12020-03-16): > That is not without review, that is all of you have no interested in > it, and I’m the maintainer of it. > So I know what I want to do. This commit was bad. Take your responsibilities. Maintaining a piece of code is not a license to produce poor code. Regards,
> 2020年3月16日 下午9:54，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-16): >> That is not without review, that is all of you have no interested in >> it, and I’m the maintainer of it. >> So I know what I want to do. > > This commit was bad. Take your responsibilities. Maintaining a piece of > code is not a license to produce poor code. Yes, You can rewrite dashdec and maintaining it, if you don’t do it, you can make it complete too, patch welcome. To Andreas, you can add xmlFree for fix the memleak wherever, I will make it complete this week. This code at here have many days, so wait it some days, it’s not busy to be removed one week. > > Regards, > > -- > Nicolas George
> 2020年3月14日 上午3:14，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Since commit e134c203 strdups of several elements of a manifest are kept > in the DASHContext; but said commit completely forgot to free these > strings again (with xmlFree()). Given that these strings are never used > at all, this commit closes this leak by reverting said commit. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/dashdec.c | 27 --------------------------- > 1 file changed, 27 deletions(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 5bbe5d3985..271202b0a5 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -122,19 +122,6 @@ 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; > > int n_videos; > struct representation **videos; > @@ -1124,26 +1111,12 @@ 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"); > > node = xmlFirstElementChild(adaptionset_node); > while (node) { > -- > 2.20.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". This is ok, but I think the comment should modify, because these variable will be used after merge this patch, I think just name revert the e134c20374ee3cbc6d04885d306b02c9871683a2 is ok. Thanks Steven Liu
diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c index 5bbe5d3985..271202b0a5 100644 --- a/libavformat/dashdec.c +++ b/libavformat/dashdec.c @@ -122,19 +122,6 @@ 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; int n_videos; struct representation **videos; @@ -1124,26 +1111,12 @@ 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"); node = xmlFirstElementChild(adaptionset_node); while (node) {
Since commit e134c203 strdups of several elements of a manifest are kept in the DASHContext; but said commit completely forgot to free these strings again (with xmlFree()). Given that these strings are never used at all, this commit closes this leak by reverting said commit. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/dashdec.c | 27 --------------------------- 1 file changed, 27 deletions(-)