diff mbox series

[FFmpeg-devel] avformat/dashdec: Don't allocate and leak strings that are never used

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 13, 2020, 7:14 p.m. UTC
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(-)

Comments

Anton Khirnov March 16, 2020, 10:46 a.m. UTC | #1
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.
Steven Liu March 16, 2020, 1:16 p.m. UTC | #2
> 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".
Nicolas George March 16, 2020, 1:19 p.m. UTC | #3
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,
Steven Liu March 16, 2020, 1:22 p.m. UTC | #4
> 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
Nicolas George March 16, 2020, 1:26 p.m. UTC | #5
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,
Steven Liu March 16, 2020, 1:29 p.m. UTC | #6
> 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
Nicolas George March 16, 2020, 1:32 p.m. UTC | #7
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,
Steven Liu March 16, 2020, 1:36 p.m. UTC | #8
> 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
Nicolas George March 16, 2020, 1:42 p.m. UTC | #9
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,
Steven Liu March 16, 2020, 1:43 p.m. UTC | #10
> 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".
Nicolas George March 16, 2020, 1:48 p.m. UTC | #11
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,
Steven Liu March 16, 2020, 1:49 p.m. UTC | #12
> 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
Nicolas George March 16, 2020, 1:54 p.m. UTC | #13
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,
Steven Liu March 16, 2020, 2 p.m. UTC | #14
> 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
Steven Liu March 29, 2020, 12:34 a.m. UTC | #15
> 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 mbox series

Patch

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) {