| Message ID | 20171230124213.87933-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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 >
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
> 在 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
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 >
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
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);
fix CID: 1426991 Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/hls.c | 1 + 1 file changed, 1 insertion(+)