| Message ID | 20170604002546.5707-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On 6/3/2017 9:25 PM, Michael Niedermayer wrote: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/utils.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index cde5849a41..feee7556ac 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -2278,6 +2278,9 @@ void avcodec_parameters_free(AVCodecParameters **ppar) > > int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src) > { > + if (src->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > + return AVERROR(EINVAL); > + > codec_parameters_reset(dst); > memcpy(dst, src, sizeof(*dst)); > > @@ -2341,6 +2344,8 @@ int avcodec_parameters_from_context(AVCodecParameters *par, > } > > if (codec->extradata) { > + if (codec->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > + return AVERROR(EINVAL); > par->extradata = av_mallocz(codec->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); > if (!par->extradata) > return AVERROR(ENOMEM); > @@ -2397,6 +2402,8 @@ int avcodec_parameters_to_context(AVCodecContext *codec, > } > > if (par->extradata) { > + if (par->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > + return AVERROR(EINVAL); > av_freep(&codec->extradata); > codec->extradata = av_mallocz(par->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); > if (!codec->extradata) > ERANGE? Or ENOMEM, the only error these functions are currently returning. EINVAL for this situation with no way to log the reason of the error is not very intuitive. The function is meant to copy the fields from one struct to the other, not really validate said fields. I see EINVAL more fit as an error for example if src or dst are NULL, something that would actually be an invalid argument. We don't currently check that, for that matter. Maybe we should... Also, unless the user calls av_max_alloc to set a value higher than INT_MAX, shouldn't av_mallocz refuse to alloc these?
On Sat, Jun 03, 2017 at 09:47:32PM -0300, James Almer wrote: > On 6/3/2017 9:25 PM, Michael Niedermayer wrote: > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/utils.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > > index cde5849a41..feee7556ac 100644 > > --- a/libavcodec/utils.c > > +++ b/libavcodec/utils.c > > @@ -2278,6 +2278,9 @@ void avcodec_parameters_free(AVCodecParameters **ppar) > > > > int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src) > > { > > + if (src->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > > + return AVERROR(EINVAL); > > + > > codec_parameters_reset(dst); > > memcpy(dst, src, sizeof(*dst)); > > > > @@ -2341,6 +2344,8 @@ int avcodec_parameters_from_context(AVCodecParameters *par, > > } > > > > if (codec->extradata) { > > + if (codec->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > > + return AVERROR(EINVAL); > > par->extradata = av_mallocz(codec->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); > > if (!par->extradata) > > return AVERROR(ENOMEM); > > @@ -2397,6 +2402,8 @@ int avcodec_parameters_to_context(AVCodecContext *codec, > > } > > > > if (par->extradata) { > > + if (par->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) > > + return AVERROR(EINVAL); > > av_freep(&codec->extradata); > > codec->extradata = av_mallocz(par->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); > > if (!codec->extradata) > > > > ERANGE? Or ENOMEM, the only error these functions are currently > returning. EINVAL for this situation with no way to log the reason of > the error is not very intuitive. The function is meant to copy the > fields from one struct to the other, not really validate said fields. > > I see EINVAL more fit as an error for example if src or dst are NULL, > something that would actually be an invalid argument. > We don't currently check that, for that matter. Maybe we should... > do you prefer ERANGE or ENOMEM ? > Also, unless the user calls av_max_alloc to set a value higher than > INT_MAX, shouldn't av_mallocz refuse to alloc these? the size is a int, the addition would overflow and be a undefined operation. I dont have a testcase that triggers this though [...]
On 6/3/2017 9:55 PM, Michael Niedermayer wrote: > On Sat, Jun 03, 2017 at 09:47:32PM -0300, James Almer wrote: >> On 6/3/2017 9:25 PM, Michael Niedermayer wrote: >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/utils.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >>> index cde5849a41..feee7556ac 100644 >>> --- a/libavcodec/utils.c >>> +++ b/libavcodec/utils.c >>> @@ -2278,6 +2278,9 @@ void avcodec_parameters_free(AVCodecParameters **ppar) >>> >>> int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src) >>> { >>> + if (src->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) >>> + return AVERROR(EINVAL); >>> + IMO, move this inside the extradata check right before the av_mallocz call, like in the two functions below. >>> codec_parameters_reset(dst); >>> memcpy(dst, src, sizeof(*dst)); >>> >>> @@ -2341,6 +2344,8 @@ int avcodec_parameters_from_context(AVCodecParameters *par, >>> } >>> >>> if (codec->extradata) { >>> + if (codec->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) >>> + return AVERROR(EINVAL); >>> par->extradata = av_mallocz(codec->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); >>> if (!par->extradata) >>> return AVERROR(ENOMEM); >>> @@ -2397,6 +2402,8 @@ int avcodec_parameters_to_context(AVCodecContext *codec, >>> } >>> >>> if (par->extradata) { >>> + if (par->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) >>> + return AVERROR(EINVAL); >>> av_freep(&codec->extradata); >>> codec->extradata = av_mallocz(par->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); >>> if (!codec->extradata) >>> >> >> ERANGE? Or ENOMEM, the only error these functions are currently >> returning. EINVAL for this situation with no way to log the reason of >> the error is not very intuitive. The function is meant to copy the >> fields from one struct to the other, not really validate said fields. >> >> I see EINVAL more fit as an error for example if src or dst are NULL, >> something that would actually be an invalid argument. >> We don't currently check that, for that matter. Maybe we should... >> > > do you prefer ERANGE or ENOMEM ? Eh, go with ERANGE i guess. > > >> Also, unless the user calls av_max_alloc to set a value higher than >> INT_MAX, shouldn't av_mallocz refuse to alloc these? > > the size is a int, the addition would overflow and be a undefined > operation. I dont have a testcase that triggers this though > > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Le quintidi 15 prairial, an CCXXV, James Almer a écrit : > On 6/3/2017 9:25 PM, Michael Niedermayer wrote: > I see EINVAL more fit as an error for example if src or dst are NULL, > something that would actually be an invalid argument. > We don't currently check that, for that matter. Maybe we should... This kind of case, I call "obviously invalid use of the API", and I think in these cases, an immediate crash is way better than an error code. The crash is already what happens here, since par and codec are immediately dereferenced. An av_assert0() would make it clearer. The same goes for every cases where passing NULL makes no sense, passing an invalid value where a small set of enumerated values / flags is expected (42 instead of SEEK_SET is an obvious example), or calling a amath function outside a simple interval. We may want to introduce something like av_assert_api() to have the failure message tell explicitly that the crash is caused by a bug in the application rather than the library. Regards,
Hi, On Sun, Jun 4, 2017 at 6:32 AM, Nicolas George <george@nsup.org> wrote: > Le quintidi 15 prairial, an CCXXV, James Almer a écrit : > > On 6/3/2017 9:25 PM, Michael Niedermayer wrote: > > I see EINVAL more fit as an error for example if src or dst are NULL, > > something that would actually be an invalid argument. > > We don't currently check that, for that matter. Maybe we should... > > This kind of case, I call "obviously invalid use of the API", and I > think in these cases, an immediate crash is way better than an error > code. The crash is already what happens here, since par and codec are > immediately dereferenced. An av_assert0() would make it clearer. > > The same goes for every cases where passing NULL makes no sense, passing > an invalid value where a small set of enumerated values / flags is > expected (42 instead of SEEK_SET is an obvious example), or calling a > amath function outside a simple interval. > > We may want to introduce something like av_assert_api() to have the > failure message tell explicitly that the crash is caused by a bug in the > application rather than the library. This reminds me of g_return_val_if_fail in glib, and in some situations it's quite sensible and helpful. Ronald
Le sextidi 16 prairial, an CCXXV, Ronald S. Bultje a écrit : > This reminds me of g_return_val_if_fail in glib, and in some situations > it's quite sensible and helpful. I think so too. But g_return_val_if_fail() has a flaw: it prints a warning and returns, at least with default builds of the library. A lot of applications developers ignore these warnings when they have no obvious adverse drawbacks. A crash is better IMHO. Note that I am only advocating a crash for invalid uses of the API that are obvious and easy to check, like: * @param foo pointer to return value, must not be NULL because it is easy for the application to wrap it and make the test: int frobnicate_or_bail(type *foo) { if (foo == NULL) { warn("I'm afraid I can't do that"); return ERROR_INVALID; } return frobnicate(foo); } My main argument is that it is almost always unnecessary: when the argument to a function must not be NULL, it is usually because the natural way of calling the function yields a non-NULL pointer. For example, in this particular case, the natural way of calling the function is probably "ret = frobnicate(&value);". The same goes for seek: when you call "seek(fd, pos, SEEK_SET);", you do not need to check that SEEK_SET is a valid value: it is. I would even argue that fd should get the same treatment, since an application doing things on a fd without knowing if it is valid is broken and may overwrite precious files (crashing will prevent that). On the other hand, the application cannot easily check if fd is seekable or if pos is valid: these conditions must cause an error return code, not a crash. Regards,
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index cde5849a41..feee7556ac 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2278,6 +2278,9 @@ void avcodec_parameters_free(AVCodecParameters **ppar) int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src) { + if (src->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) + return AVERROR(EINVAL); + codec_parameters_reset(dst); memcpy(dst, src, sizeof(*dst)); @@ -2341,6 +2344,8 @@ int avcodec_parameters_from_context(AVCodecParameters *par, } if (codec->extradata) { + if (codec->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) + return AVERROR(EINVAL); par->extradata = av_mallocz(codec->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); if (!par->extradata) return AVERROR(ENOMEM); @@ -2397,6 +2402,8 @@ int avcodec_parameters_to_context(AVCodecContext *codec, } if (par->extradata) { + if (par->extradata_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) + return AVERROR(EINVAL); av_freep(&codec->extradata); codec->extradata = av_mallocz(par->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); if (!codec->extradata)
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/utils.c | 7 +++++++ 1 file changed, 7 insertions(+)