| Message ID | d040376f-72bc-e232-167b-7b31ca4e269a@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > This should reduce the impact of a demuxer (or API user) setting bogus > codec parameters. > > This seems rather noisy and doesn't really solve anything, does it? Decoders still need to validate values instead of blindly trusting them, and this just hides some problems in these decoders, instead of fixing them there. API users of avcodec would not fill AVCodecParameters, they would fill a codec context directly. - Hendrik
On Tue, Oct 25, 2016 at 01:50:47AM +0200, Andreas Cadhalpun wrote: > This should reduce the impact of a demuxer (or API user) setting bogus > codec parameters. > > Suggested-by: wm4 > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavcodec/utils.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 81 insertions(+), 1 deletion(-) This adds some warnings: libavcodec/utils.c: In function ‘avcodec_parameters_to_context’: libavcodec/utils.c:4256:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] libavcodec/utils.c:4261:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] libavcodec/utils.c:4266:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] libavcodec/utils.c:4271:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] libavcodec/utils.c:4276:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] i guess thats enum (un)signednes ... [...]
On Tue, 25 Oct 2016 09:47:29 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun > <andreas.cadhalpun@googlemail.com> wrote: > > This should reduce the impact of a demuxer (or API user) setting bogus > > codec parameters. > > > > > > This seems rather noisy and doesn't really solve anything, does it? > Decoders still need to validate values instead of blindly trusting > them, and this just hides some problems in these decoders, instead of > fixing them there. API users of avcodec would not fill > AVCodecParameters, they would fill a codec context directly. You could also argue that the demuxer shouldn't return invalid parameters either. How about this: always convert the params to a temporary codecpar, and provide a function to determine the validity of a codecpar. This way the check could be done in multiple places without duplicating the code needed for it.
On Tue, Oct 25, 2016 at 2:39 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 25 Oct 2016 09:47:29 +0200 > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun >> <andreas.cadhalpun@googlemail.com> wrote: >> > This should reduce the impact of a demuxer (or API user) setting bogus >> > codec parameters. >> > >> > >> >> This seems rather noisy and doesn't really solve anything, does it? >> Decoders still need to validate values instead of blindly trusting >> them, and this just hides some problems in these decoders, instead of >> fixing them there. API users of avcodec would not fill >> AVCodecParameters, they would fill a codec context directly. > > You could also argue that the demuxer shouldn't return invalid > parameters either. It should not, but this patch does not address this. There is various combinations of component usage that are possible, and in fact are used in the real world: avformat -> avcodec other demuxer -> avcodec avformat -> other decoder This patch only addresses the first case, and only if you actually use this function (which I for example do not, since I have an abstraction layer in between, so I never have AVCodecParameters and AVCodecContext in the same function). So in short, it just doesn't fix much, and you can still get invalid output from avformat, and potentially still undefined behavior in avcodec if its fed those values through other means. > > How about this: always convert the params to a temporary codecpar, and > provide a function to determine the validity of a codecpar. This way > the check could be done in multiple places without duplicating the code > needed for it. That still sounds odd, although slightly better. At the very least it should be a dedicated function that checks the values in a key place, say you want to check params that are fed to a decoder, then call this check in avcodec_open, because thats something everyone has to call to use avcodec. - Hendrik
On 25.10.2016 13:43, Michael Niedermayer wrote: > On Tue, Oct 25, 2016 at 01:50:47AM +0200, Andreas Cadhalpun wrote: >> This should reduce the impact of a demuxer (or API user) setting bogus >> codec parameters. >> >> Suggested-by: wm4 >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavcodec/utils.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 81 insertions(+), 1 deletion(-) > > This adds some warnings: > > libavcodec/utils.c: In function ‘avcodec_parameters_to_context’: > libavcodec/utils.c:4256:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > libavcodec/utils.c:4261:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > libavcodec/utils.c:4266:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > libavcodec/utils.c:4271:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > libavcodec/utils.c:4276:9: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > > i guess thats enum (un)signednes ... I'll just cast the enums to unsigned and check for > *_NB. Best regards, Andreas
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 87de15f..9977ffd 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -4227,8 +4227,20 @@ int avcodec_parameters_to_context(AVCodecContext *codec, codec->codec_id = par->codec_id; codec->codec_tag = par->codec_tag; + if (par->bit_rate < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid bit rate %"PRId64"\n", par->bit_rate); + return AVERROR(EINVAL); + } codec->bit_rate = par->bit_rate; + if (par->bits_per_coded_sample < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid bits per coded sample %d\n", par->bits_per_coded_sample); + return AVERROR(EINVAL); + } codec->bits_per_coded_sample = par->bits_per_coded_sample; + if (par->bits_per_raw_sample < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid bits per raw sample %d\n", par->bits_per_raw_sample); + return AVERROR(EINVAL); + } codec->bits_per_raw_sample = par->bits_per_raw_sample; codec->profile = par->profile; codec->level = par->level; @@ -4236,42 +4248,110 @@ int avcodec_parameters_to_context(AVCodecContext *codec, switch (par->codec_type) { case AVMEDIA_TYPE_VIDEO: codec->pix_fmt = par->format; + if ( (par->width || par->height) && av_image_check_size(par->width, par->height, 0, codec) < 0) + return AVERROR(EINVAL); codec->width = par->width; codec->height = par->height; codec->field_order = par->field_order; + if (par->color_range < 0 || par->color_range > AVCOL_RANGE_NB) { + av_log(codec, AV_LOG_ERROR, "Invalid color range %d\n", par->color_range); + return AVERROR(EINVAL); + } codec->color_range = par->color_range; + if (par->color_primaries < 0 || par->color_primaries > AVCOL_PRI_NB) { + av_log(codec, AV_LOG_ERROR, "Invalid color primaries %d\n", par->color_primaries); + return AVERROR(EINVAL); + } codec->color_primaries = par->color_primaries; + if (par->color_trc < 0 || par->color_trc > AVCOL_TRC_NB) { + av_log(codec, AV_LOG_ERROR, "Invalid color transfer characteristics %d\n", par->color_trc); + return AVERROR(EINVAL); + } codec->color_trc = par->color_trc; + if (par->color_space < 0 || par->color_space > AVCOL_SPC_NB) { + av_log(codec, AV_LOG_ERROR, "Invalid color space %d\n", par->color_space); + return AVERROR(EINVAL); + } codec->colorspace = par->color_space; + if (par->chroma_location < 0 || par->chroma_location > AVCHROMA_LOC_NB) { + av_log(codec, AV_LOG_ERROR, "Invalid chroma location %d\n", par->chroma_location); + return AVERROR(EINVAL); + } codec->chroma_sample_location = par->chroma_location; + if (par->sample_aspect_ratio.num < 0 || par->sample_aspect_ratio.den < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid sample aspect ratio %d/%d\n", + par->sample_aspect_ratio.num, par->sample_aspect_ratio.den); + return AVERROR(EINVAL); + } codec->sample_aspect_ratio = par->sample_aspect_ratio; + if (par->video_delay < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid video delay %d\n", par->video_delay); + return AVERROR(EINVAL); + } codec->has_b_frames = par->video_delay; break; case AVMEDIA_TYPE_AUDIO: + if (par->format < -1 || par->format > AV_SAMPLE_FMT_NB) { + av_log(codec, AV_LOG_ERROR, "Invalid sample format %d\n", par->format); + return AVERROR(EINVAL); + } codec->sample_fmt = par->format; codec->channel_layout = par->channel_layout; + if (par->channels < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid number of channels %d\n", par->channels); + return AVERROR(EINVAL); + } codec->channels = par->channels; + if (par->sample_rate < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid sample rate %d\n", par->sample_rate); + return AVERROR(EINVAL); + } codec->sample_rate = par->sample_rate; + if (par->block_align < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid block align %d\n", par->block_align); + return AVERROR(EINVAL); + } codec->block_align = par->block_align; + if (par->frame_size < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid frame size %d\n", par->frame_size); + return AVERROR(EINVAL); + } codec->frame_size = par->frame_size; + if (par->initial_padding < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid initial padding %d\n", par->initial_padding); + return AVERROR(EINVAL); + } codec->delay = codec->initial_padding = par->initial_padding; + if (par->trailing_padding < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid trailing padding %d\n", par->trailing_padding); + return AVERROR(EINVAL); + } codec->trailing_padding = par->trailing_padding; + if (par->seek_preroll < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid seek preroll %d\n", par->seek_preroll); + return AVERROR(EINVAL); + } codec->seek_preroll = par->seek_preroll; break; case AVMEDIA_TYPE_SUBTITLE: + if ((par->width || par->height) && av_image_check_size(par->width, par->height, 0, codec) < 0) + return AVERROR(EINVAL); codec->width = par->width; codec->height = par->height; break; } - if (par->extradata) { + if (par->extradata_size > 0) { av_freep(&codec->extradata); codec->extradata = av_mallocz(par->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); if (!codec->extradata) return AVERROR(ENOMEM); memcpy(codec->extradata, par->extradata, par->extradata_size); codec->extradata_size = par->extradata_size; + } else if (par->extradata_size < 0) { + av_log(codec, AV_LOG_ERROR, "Invalid extradata size %d", par->extradata_size); + return AVERROR(EINVAL); } return 0;
This should reduce the impact of a demuxer (or API user) setting bogus codec parameters. Suggested-by: wm4 Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/utils.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)