[FFmpeg-devel] avformat/hls: release mem resource to fix memleak

Submitted by Steven Liu on Dec. 30, 2017, 12:42 p.m.

Details

Message ID 20171230124213.87933-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu Dec. 30, 2017, 12:42 p.m.
fix CID: 1426991

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hls.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Derek Buitenhuis Dec. 30, 2017, 4:26 p.m.
On 12/30/2017 12:42 PM, Steven Liu wrote:
>          av_opt_get(v->input, "http_version", AV_OPT_SEARCH_CHILDREN, &http_version_opt);
>          c->http_multiple = http_version_opt && strncmp((const char *)http_version_opt, "1.1", 3) == 0;
> +        av_free(http_version_opt);

Looks OK, but the return value for av_opt_get should also be checked. It can
fail with, for example, AVERROR(ENOMEM).

- Derek
Nicolas George Dec. 30, 2017, 4:31 p.m.
Derek Buitenhuis (2017-12-30):
> On 12/30/2017 12:42 PM, Steven Liu wrote:
> >          av_opt_get(v->input, "http_version", AV_OPT_SEARCH_CHILDREN, &http_version_opt);
> >          c->http_multiple = http_version_opt && strncmp((const char *)http_version_opt, "1.1", 3) == 0;
> > +        av_free(http_version_opt);
> 
> Looks OK, but the return value for av_opt_get should also be checked. It can
> fail with, for example, AVERROR(ENOMEM).

Does it really matter? If av_opt_get() fails for any reason,
http_multiple will just be false, which would let the processing
continue, only in a slightly degraded manner that was the norm a few
months ago.

Regards,
Derek Buitenhuis Dec. 30, 2017, 4:34 p.m.
On 12/30/2017 4:31 PM, Nicolas George wrote:
> Does it really matter? If av_opt_get() fails for any reason,
> http_multiple will just be false, which would let the processing
> continue, only in a slightly degraded manner that was the norm a few
> months ago.

I contend that checking errors should *always* be done, as a matter of
good practice, not whenever it "seems needed". Especially when it can
be a alloc failure, leading, in this case to calling:

    strncmp((const char *)NULL, http_version_opt, "1.1", 3)

if it fails.

- Derek
Aman Gupta Dec. 30, 2017, 5:50 p.m.
On Sat, Dec 30, 2017 at 8:34 AM Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> On 12/30/2017 4:31 PM, Nicolas George wrote:
> > Does it really matter? If av_opt_get() fails for any reason,
> > http_multiple will just be false, which would let the processing
> > continue, only in a slightly degraded manner that was the norm a few
> > months ago.
>
> I contend that checking errors should *always* be done, as a matter of
> good practice, not whenever it "seems needed". Especially when it can
> be a alloc failure, leading, in this case to calling:
>
>     strncmp((const char *)NULL, http_version_opt, "1.1", 3)
>
> if it fails.


There is already a check in place to prevent strncmp from being called with
NULL.

Aman


>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Derek Buitenhuis Dec. 30, 2017, 10:14 p.m.
On 12/30/2017 5:50 PM, Aman Gupta wrote:
> There is already a check in place to prevent strncmp from being called with
> NULL.

The point before still holds.

Are people really arguing against consistent use of error checking? Inconsistent
standards of error checking are how bugs and security issues happen.

- Derek
Steven Liu Dec. 31, 2017, 1:01 a.m.
> 在 2017年12月31日,上午6:14,Derek Buitenhuis <derek.buitenhuis@gmail.com> 写道:
> 
>> On 12/30/2017 5:50 PM, Aman Gupta wrote:
>> There is already a check in place to prevent strncmp from being called with
>> NULL.
> 
> The point before still holds.
> 
> Are people really arguing against consistent use of error checking? Inconsistent
> standards of error checking are how bugs and security issues happen.
> 

ok, patch will update next year :D
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Aman Gupta Dec. 31, 2017, 5:21 a.m.
On Sat, Dec 30, 2017 at 2:14 PM Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> On 12/30/2017 5:50 PM, Aman Gupta wrote:
> > There is already a check in place to prevent strncmp from being called
> with
> > NULL.
>
> The point before still holds.
>
> Are people really arguing against consistent use of error checking?
> Inconsistent
> standards of error checking are how bugs and security issues happen.


I really don't think it makes any sense in this case, since it is expected
that the av_opt_get will fail on non-http io contexts. It doesn't matter if
the failure is due to AVERROR_OPTION_NOT_FOUND or AVERROR(ENOMEM). The only
thing that matters is whether http_version_opt is set or remains NULL.

Anyway, I don't feel strongly about this. If you think the extra
instructions to store and compare the return value make this code cleaner
or more secure, so be it. I will send a patch with the requested changes.

Aman


>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Derek Buitenhuis Dec. 31, 2017, 6:29 a.m.
On 12/31/2017 5:21 AM, Aman Gupta wrote:
> I really don't think it makes any sense in this case, since it is expected
> that the av_opt_get will fail on non-http io contexts. It doesn't matter if
> the failure is due to AVERROR_OPTION_NOT_FOUND or AVERROR(ENOMEM). The only
> thing that matters is whether http_version_opt is set or remains NULL.

A behavioral change that may occur due to OOM rather than failing is not
very fun to debug.

Anyway, part of my point was that using error checks inconsistently often
leads to sloppiness elsewhere, because, well, checks are not always used,
and judgement on when to use them may not always be right, or the underlying
implementation of the unchecked function may change, such that it now does
matter (assumptions of the API being checked as per documentation, etc.).
There are long term benefits to a culture of always checking, IMO. I'm
sure people will reply to disagree with me, of course.

> Anyway, I don't feel strongly about this. If you think the extra
> instructions to store and compare the return value make this code cleaner
> or more secure, so be it. I will send a patch with the requested changes.

OK. (Fretting about extra instructions for return value checks in playlist
code is a tad silly...)

Sorry for the rant -- this sort of lax API use has been a pain in the
past when trying to change around implementation details, where API
users have neglected to check return values.

- Derek

Patch hide | download patch | download mbox

diff --git a/libavformat/hls.c b/libavformat/hls.c
index dccc7c7dd2..9918d1af74 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -1475,6 +1475,7 @@  reload:
         uint8_t *http_version_opt = NULL;
         av_opt_get(v->input, "http_version", AV_OPT_SEARCH_CHILDREN, &http_version_opt);
         c->http_multiple = http_version_opt && strncmp((const char *)http_version_opt, "1.1", 3) == 0;
+        av_free(http_version_opt);
     }
 
     seg = next_segment(v);