| Message ID | 54f9a7a9-4917-86aa-bd91-4872552e8282@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Nov 19, 2016 at 02:04:44PM +0100, Andreas Cadhalpun wrote: > On 19.11.2016 02:14, Michael Niedermayer wrote: > > On Fri, Nov 18, 2016 at 10:35:29PM +0100, Andreas Cadhalpun wrote: > >> On 18.11.2016 02:40, Michael Niedermayer wrote: > >>> On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote: > >>>> + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { > >>>> + av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size); > >>> > >>> i think this and possibly others should be a hard failure > >>> or why would a invalid extradata_size be ok ? > >> > >> The decoder might still be able to work. > >> What would be the advantage of a hard failure? > > > > the advantage of stoping clearly invalid data early > > The decoder runs on a seperate machine with ffm possibly. The file > > just gets demuxed and sent over the network. The warning hinting to > > the issue is on the sending side. The decoder failure at the receiver > > i didnt try but if iam not mixing things up that would be confusing > > neither side would fully understand whats going on without the other > > OK, I changed the extradata_size case to an error. > Which others do you think should be changed, too? why do you want to accept any invalid values ? ffm files should only be generated by FFmpeg, why should FFmpeg generate invalid files or what is the use case where this occurs ? It feels like iam missing something here ... [...]
On 19.11.2016 16:28, Michael Niedermayer wrote: > On Sat, Nov 19, 2016 at 02:04:44PM +0100, Andreas Cadhalpun wrote: >> On 19.11.2016 02:14, Michael Niedermayer wrote: >>> On Fri, Nov 18, 2016 at 10:35:29PM +0100, Andreas Cadhalpun wrote: >>>> On 18.11.2016 02:40, Michael Niedermayer wrote: >>>>> On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote: >>>>>> + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { >>>>>> + av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size); >>>>> >>>>> i think this and possibly others should be a hard failure >>>>> or why would a invalid extradata_size be ok ? >>>> >>>> The decoder might still be able to work. >>>> What would be the advantage of a hard failure? >>> >>> the advantage of stoping clearly invalid data early >>> The decoder runs on a seperate machine with ffm possibly. The file >>> just gets demuxed and sent over the network. The warning hinting to >>> the issue is on the sending side. The decoder failure at the receiver >>> i didnt try but if iam not mixing things up that would be confusing >>> neither side would fully understand whats going on without the other >> >> OK, I changed the extradata_size case to an error. >> Which others do you think should be changed, too? > > why do you want to accept any invalid values ? > ffm files should only be generated by FFmpeg, why should FFmpeg > generate invalid files or what is the use case where this occurs ? > It feels like iam missing something here ... A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2] convinced me that it should be avoided. Best regards, Andreas 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202783.html 2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202785.html
On Sat, Nov 19, 2016 at 05:30:59PM +0100, Andreas Cadhalpun wrote: > On 19.11.2016 16:28, Michael Niedermayer wrote: > > On Sat, Nov 19, 2016 at 02:04:44PM +0100, Andreas Cadhalpun wrote: > >> On 19.11.2016 02:14, Michael Niedermayer wrote: > >>> On Fri, Nov 18, 2016 at 10:35:29PM +0100, Andreas Cadhalpun wrote: > >>>> On 18.11.2016 02:40, Michael Niedermayer wrote: > >>>>> On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote: > >>>>>> + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { > >>>>>> + av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size); > >>>>> > >>>>> i think this and possibly others should be a hard failure > >>>>> or why would a invalid extradata_size be ok ? > >>>> > >>>> The decoder might still be able to work. > >>>> What would be the advantage of a hard failure? > >>> > >>> the advantage of stoping clearly invalid data early > >>> The decoder runs on a seperate machine with ffm possibly. The file > >>> just gets demuxed and sent over the network. The warning hinting to > >>> the issue is on the sending side. The decoder failure at the receiver > >>> i didnt try but if iam not mixing things up that would be confusing > >>> neither side would fully understand whats going on without the other > >> > >> OK, I changed the extradata_size case to an error. > >> Which others do you think should be changed, too? > > > > why do you want to accept any invalid values ? > > ffm files should only be generated by FFmpeg, why should FFmpeg > > generate invalid files or what is the use case where this occurs ? > > It feels like iam missing something here ... > > A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2] > convinced me that it should be avoided. I think there are 2 different things here unavailable data, like a bitrate of 0 and wrong data like a negative bitrate We do not accept wrong values in general, why should it be different here ? ffm files are AFAIK generally temporary (on disk fifo) files generated by the current muxer. Is our muxer buggy ? If it is not where would invalid values come from ? [...]
2016-11-19 17:30 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2] > convinced me that it should be avoided. I believe that ffm should (or at least can) indeed be treated differently from all other containers. > 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202783.html > 2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/202785.html Carl Eugen
On 20.11.2016 13:42, Carl Eugen Hoyos wrote: > 2016-11-19 17:30 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > >> A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2] >> convinced me that it should be avoided. > > I believe that ffm should (or at least can) indeed be treated differently > from all other containers. OK, ffm is a special case, but I'm also interested in the general case. Currently many demuxers silently accept wrong (i.e. negative) values for channels, bit_rate, block_align and so on. I'd like to fix that, so the question is now, how? There are a few possibilities: a) error out for negative values and also for zero b) error out for negative values, silently accept zero c) warn for negative values and also for zero d) warn for negative values, silently accept zero e) something else Is there a consensus on which way is best? Best regards, Andreas
2016-11-20 20:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > On 20.11.2016 13:42, Carl Eugen Hoyos wrote: >> 2016-11-19 17:30 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> >>> A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2] >>> convinced me that it should be avoided. >> >> I believe that ffm should (or at least can) indeed be treated differently >> from all other containers. > > OK, ffm is a special case, but I'm also interested in the general case. > > Currently many demuxers silently accept wrong (i.e. negative) values > for channels, bit_rate, block_align and so on. I'd like to fix that, > so the question is now, how? > > There are a few possibilities: > a) error out for negative values and also for zero Only if -fstrict strict was set. > b) error out for negative values, silently accept zero Only if -fstrict strict was set. > c) warn for negative values and also for zero > d) warn for negative values, silently accept zero I obviously cannot stop you if you feel this should be done, but note that users will report regressions "warnings are shown for playable files". > e) something else Broken files exist and FFmpeg should play them if reasonable. Carl Eugen
On Sun, Nov 20, 2016 at 08:40:24PM +0100, Andreas Cadhalpun wrote: > On 20.11.2016 13:42, Carl Eugen Hoyos wrote: > > 2016-11-19 17:30 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > > > >> A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2] > >> convinced me that it should be avoided. > > > > I believe that ffm should (or at least can) indeed be treated differently > > from all other containers. > > OK, ffm is a special case, but I'm also interested in the general case. > > Currently many demuxers silently accept wrong (i.e. negative) values > for channels, bit_rate, block_align and so on. I'd like to fix that, > so the question is now, how? > > There are a few possibilities: > a) error out for negative values and also for zero > b) error out for negative values, silently accept zero > c) warn for negative values and also for zero > d) warn for negative values, silently accept zero > e) something else > > Is there a consensus on which way is best? I think this depends on the format and what exists in real world files If X is allowed by the spec, clearly no error or warning should be produced for it If X is not allowed by the spec but occurs in some file then no error should occur by default but likely a warning. More strict compliance options can change this. If X does not work (demuxer failing in some form) then it should error out Theres quite a bit between these and theres the problem of not knowing spec and file existence easily [...]
On 20.11.2016 21:02, Carl Eugen Hoyos wrote: > 2016-11-20 20:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> Currently many demuxers silently accept wrong (i.e. negative) values >> for channels, bit_rate, block_align and so on. I'd like to fix that, >> so the question is now, how? >> >> There are a few possibilities: >> a) error out for negative values and also for zero > > Only if -fstrict strict was set. > >> b) error out for negative values, silently accept zero > > Only if -fstrict strict was set. So you have no preference between accepting zero or not? I tend to silently accept zero, because I'd like a consistent solution and in some cases rejecting zero causes FATE failures. If a zero causes SIGFPE crashes, it obviously needs to be rejected with an error. I also like the idea to only error out if strict is set and otherwise only warn. >> c) warn for negative values and also for zero >> d) warn for negative values, silently accept zero > > I obviously cannot stop you if you feel this should > be done, but note that users will report regressions > "warnings are shown for playable files". Isn't that a good thing? Because either our demuxer has a bug and should be fixed, or some other tool created broken files, and if ffmpeg informs it's users about that, they can try to get that tool fixed. >> e) something else > > Broken files exist and FFmpeg should play them if > reasonable. I agree in principle. Best regards, Andreas
On 21.11.2016 03:13, Michael Niedermayer wrote: > On Sun, Nov 20, 2016 at 08:40:24PM +0100, Andreas Cadhalpun wrote: >> Currently many demuxers silently accept wrong (i.e. negative) values >> for channels, bit_rate, block_align and so on. I'd like to fix that, >> so the question is now, how? >> >> There are a few possibilities: >> a) error out for negative values and also for zero >> b) error out for negative values, silently accept zero >> c) warn for negative values and also for zero >> d) warn for negative values, silently accept zero >> e) something else >> >> Is there a consensus on which way is best? > > I think this depends on the format and what exists in real world files > > If X is allowed by the spec, clearly no error or warning should be > produced for it > > If X is not allowed by the spec but occurs in some file then no error > should occur by default but likely a warning. More strict compliance > options can change this. > > If X does not work (demuxer failing in some form) then it should error > out > > > Theres quite a bit between these and theres the problem of not knowing > spec and file existence easily In general that sounds reasonable, but what does it mean in practice? For example what should be done about overflows, e.g. when calculating the bit rate? Does this count as demuxer failing? And what should be done if the spec says a field is unsigned, but our framework only supports the signed variant? Best regards, Andreas
2016-11-22 0:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > For example what should be done about overflows, e.g. when > calculating the bit rate? Does this count as demuxer failing? I don't understand this question: There are formats for which we don't know the specification (or it may not exist): Of course we always want to fix all undefined behaviour, all crashes and similar - this is not related to the specifications in question. FFmpeg should never by default refuse to decode media files that can be decoded and it should never stop reading such files. > And what should be done if the spec says a field is unsigned, > but our framework only supports the signed variant? Is there a sample for which this makes a difference? If yes, we should try to fix it. Carl Eugen
2016-11-22 0:06 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > On 20.11.2016 21:02, Carl Eugen Hoyos wrote: >> 2016-11-20 20:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>> Currently many demuxers silently accept wrong (i.e. negative) values >>> for channels, bit_rate, block_align and so on. I'd like to fix that, >>> so the question is now, how? >>> >>> There are a few possibilities: >>> a) error out for negative values and also for zero >> >> Only if -fstrict strict was set. >> >>> b) error out for negative values, silently accept zero >> >> Only if -fstrict strict was set. > > So you have no preference between accepting zero or not? Sorry: By default, FFmpeg should not error out because of an invalid value in some field, neither if it is zero although it should be positive nor when it is negative. > I tend to silently accept zero, because I'd like a > consistent solution and in some cases rejecting zero > causes FATE failures. If there were a bug, you should of course fix fate, fate alone is generally no argument in favour or against a patch. > If a zero causes SIGFPE crashes, it obviously needs to > be rejected with an error. The crash has to be fixed. If a simple fix is possible that allows to decode such samples, it should be preferred over a fix that doesn't allow it. > I also like the idea to only error out if strict is set > and otherwise only warn. > >>> c) warn for negative values and also for zero >>> d) warn for negative values, silently accept zero >> >> I obviously cannot stop you if you feel this should >> be done, but note that users will report regressions >> "warnings are shown for playable files". > > Isn't that a good thing? I tried to explain above why it is not necessarily a good thing. > Because either our demuxer has a bug and should be fixed, > or some other tool created broken files, and if ffmpeg > informs it's users about that, they can try to get that > tool fixed. (Träum weiter) Seriously: We have an uncountable amount of files that do not respect some "specification", many of them apparently by intention, many written by applications that do not exist since a long time, and some written by applications that have fixed the issue meanwhile: Of course it can be a (very) good thing to print warnings but in general, we should try to fix a few of the thousands of known bugs in FFmpeg and not spend infinite time on possible issues in other applications. Sorry if I just misunderstand you. Carl Eugen
On 22.11.2016 01:27, Carl Eugen Hoyos wrote: > 2016-11-22 0:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > >> For example what should be done about overflows, e.g. when >> calculating the bit rate? Does this count as demuxer failing? > > I don't understand this question: > There are formats for which we don't know the specification (or > it may not exist): Of course we always want to fix all undefined > behaviour, all crashes and similar - this is not related to the > specifications in question. The fundamental problem is that multiplying two int32_t variables does not always fit in an int32_t. If the result of the multiplication is too large, it wraps around into a negative value, that can cause problems later on. In a way this is similar to reading a negative value from a file, and could be handled with a warning by default, however it could also indicate a limitation in our framework and just setting the value to 0 in case of overflow could cause the file to be decoded incorrectly. Thus it might be better to always error out in such cases. > FFmpeg should never by default refuse to decode media files > that can be decoded and it should never stop reading such files. 'Never' is certainly not correct, since e.g. the ffm muxer is a special case, as you agreed. Also there are currently many cases in which FFmpeg errors out, even though it could try to ignore the problem. >> And what should be done if the spec says a field is unsigned, >> but our framework only supports the signed variant? > > Is there a sample for which this makes a difference? If yes, we > should try to fix it. There are certainly fuzzed samples, where this is an issue. How do you propose to fix this? Best regards, Andreas
On 22.11.2016 01:35, Carl Eugen Hoyos wrote: > 2016-11-22 0:06 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> I tend to silently accept zero, because I'd like a >> consistent solution and in some cases rejecting zero >> causes FATE failures. > > If there were a bug, you should of course fix fate, fate > alone is generally no argument in favour or against a patch. The cases I mean don't look like bugs, just like cases, where it is expected that some files set certain codec parameters to 0. >> Because either our demuxer has a bug and should be fixed, >> or some other tool created broken files, and if ffmpeg >> informs it's users about that, they can try to get that >> tool fixed. > > (Träum weiter) > Seriously: We have an uncountable amount of files that > do not respect some "specification", many of them apparently > by intention, many written by applications that do not exist > since a long time, and some written by applications that have > fixed the issue meanwhile: Of course it can be a (very) good > thing to print warnings but in general, we should try to fix a > few of the thousands of known bugs in FFmpeg and not > spend infinite time on possible issues in other applications. > > Sorry if I just misunderstand you. I didn't want to suggest that we fix other applications, just that users of ffmpeg can realize there is a problem in another application, if files produced by that application trigger a warning in ffmpeg. (Of course this also applies to ffmpeg's muxers.) Best regards, Andreas
From 6ef854846a12fe054b81f36ff7ba50b82c4bca34 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> Date: Thu, 17 Nov 2016 00:04:57 +0100 Subject: [PATCH] ffmdec: sanitize codec parameters A negative extradata size for example gets passed to memcpy in avcodec_parameters_from_context causing a segmentation fault. Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavformat/ffmdec.c | 107 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 43 deletions(-) diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c index 6b09be2..d8d5e36 100644 --- a/libavformat/ffmdec.c +++ b/libavformat/ffmdec.c @@ -21,6 +21,7 @@ #include <stdint.h> +#include "libavutil/imgutils.h" #include "libavutil/internal.h" #include "libavutil/intreadwrite.h" #include "libavutil/intfloat.h" @@ -28,6 +29,7 @@ #include "libavutil/avassert.h" #include "libavutil/avstring.h" #include "libavutil/pixdesc.h" +#include "libavcodec/internal.h" #include "avformat.h" #include "internal.h" #include "ffm.h" @@ -277,6 +279,13 @@ static int ffm_append_recommended_configuration(AVStream *st, char **conf) return 0; } +#define SANITIZE_PARAMETER(parameter, name, check, default) { \ + if (check) { \ + av_log(codec, AV_LOG_WARNING, "Invalid " name " %d\n", codec->parameter); \ + codec->parameter = default; \ + } \ +} + static int ffm2_read_header(AVFormatContext *s) { FFMContext *ffm = s->priv_data; @@ -346,23 +355,30 @@ static int ffm2_read_header(AVFormatContext *s) } codec->codec_type = avio_r8(pb); if (codec->codec_type != codec_desc->type) { - av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n", + av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n", codec_desc->type, codec->codec_type); - codec->codec_id = AV_CODEC_ID_NONE; - codec->codec_type = AVMEDIA_TYPE_UNKNOWN; - goto fail; + codec->codec_type = codec_desc->type; } codec->bit_rate = avio_rb32(pb); + if (codec->bit_rate < 0) { + av_log(codec, AV_LOG_WARNING, "Invalid bit rate %"PRId64"\n", codec->bit_rate); + codec->bit_rate = 0; + } codec->flags = avio_rb32(pb); codec->flags2 = avio_rb32(pb); codec->debug = avio_rb32(pb); if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) { int size = avio_rb32(pb); - codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); - if (!codec->extradata) - return AVERROR(ENOMEM); - codec->extradata_size = size; - avio_read(pb, codec->extradata, size); + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { + av_log(s, AV_LOG_ERROR, "Invalid extradata size %d\n", size); + return AVERROR_INVALIDDATA; + } else { + codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!codec->extradata) + return AVERROR(ENOMEM); + codec->extradata_size = size; + avio_read(pb, codec->extradata, size); + } } break; case MKBETAG('S', 'T', 'V', 'I'): @@ -372,21 +388,21 @@ static int ffm2_read_header(AVFormatContext *s) } codec->time_base.num = avio_rb32(pb); codec->time_base.den = avio_rb32(pb); - if (codec->time_base.num <= 0 || codec->time_base.den <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid time base %d/%d\n", + if (codec->time_base.num < 0 || codec->time_base.den <= 0) { + av_log(s, AV_LOG_WARNING, "Invalid time base %d/%d\n", codec->time_base.num, codec->time_base.den); - ret = AVERROR_INVALIDDATA; - goto fail; + codec->time_base.num = 0; + codec->time_base.den = 1; } codec->width = avio_rb16(pb); codec->height = avio_rb16(pb); + if (av_image_check_size(codec->width, codec->height, 0, s) < 0) { + codec->width = 0; + codec->height = 0; + } codec->gop_size = avio_rb16(pb); codec->pix_fmt = avio_rb32(pb); - if (!av_pix_fmt_desc_get(codec->pix_fmt)) { - av_log(s, AV_LOG_ERROR, "Invalid pix fmt id: %d\n", codec->pix_fmt); - codec->pix_fmt = AV_PIX_FMT_NONE; - goto fail; - } + SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt != AV_PIX_FMT_NONE && !av_pix_fmt_desc_get(codec->pix_fmt), AV_PIX_FMT_NONE) codec->qmin = avio_r8(pb); codec->qmax = avio_r8(pb); codec->max_qdiff = avio_r8(pb); @@ -432,13 +448,11 @@ static int ffm2_read_header(AVFormatContext *s) goto fail; } codec->sample_rate = avio_rb32(pb); - if (codec->sample_rate <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", codec->sample_rate); - ret = AVERROR_INVALIDDATA; - goto fail; - } + SANITIZE_PARAMETER(sample_rate, "sample rate", codec->sample_rate < 0, 0) codec->channels = avio_rl16(pb); + SANITIZE_PARAMETER(channels, "number of channels", codec->channels < 0, 0) codec->frame_size = avio_rl16(pb); + SANITIZE_PARAMETER(frame_size, "frame size", codec->frame_size < 0, 0) break; case MKBETAG('C', 'P', 'R', 'V'): if (f_cprv++) { @@ -563,13 +577,15 @@ static int ffm_read_header(AVFormatContext *s) } codec->codec_type = avio_r8(pb); /* codec_type */ if (codec->codec_type != codec_desc->type) { - av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n", + av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n", codec_desc->type, codec->codec_type); - codec->codec_id = AV_CODEC_ID_NONE; - codec->codec_type = AVMEDIA_TYPE_UNKNOWN; - goto fail; + codec->codec_type = codec_desc->type; } codec->bit_rate = avio_rb32(pb); + if (codec->bit_rate < 0) { + av_log(codec, AV_LOG_WARNING, "Invalid bit rate %"PRId64"\n", codec->bit_rate); + codec->bit_rate = 0; + } codec->flags = avio_rb32(pb); codec->flags2 = avio_rb32(pb); codec->debug = avio_rb32(pb); @@ -579,19 +595,20 @@ static int ffm_read_header(AVFormatContext *s) codec->time_base.num = avio_rb32(pb); codec->time_base.den = avio_rb32(pb); if (codec->time_base.num <= 0 || codec->time_base.den <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid time base %d/%d\n", + av_log(s, AV_LOG_WARNING, "Invalid time base %d/%d\n", codec->time_base.num, codec->time_base.den); - goto fail; + codec->time_base.num = 0; + codec->time_base.den = 1; } codec->width = avio_rb16(pb); codec->height = avio_rb16(pb); + if (av_image_check_size(codec->width, codec->height, 0, s) < 0) { + codec->width = 0; + codec->height = 0; + } codec->gop_size = avio_rb16(pb); codec->pix_fmt = avio_rb32(pb); - if (!av_pix_fmt_desc_get(codec->pix_fmt)) { - av_log(s, AV_LOG_ERROR, "Invalid pix fmt id: %d\n", codec->pix_fmt); - codec->pix_fmt = AV_PIX_FMT_NONE; - goto fail; - } + SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt != AV_PIX_FMT_NONE && !av_pix_fmt_desc_get(codec->pix_fmt), AV_PIX_FMT_NONE) codec->qmin = avio_r8(pb); codec->qmax = avio_r8(pb); codec->max_qdiff = avio_r8(pb); @@ -633,23 +650,27 @@ static int ffm_read_header(AVFormatContext *s) break; case AVMEDIA_TYPE_AUDIO: codec->sample_rate = avio_rb32(pb); - if (codec->sample_rate <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", codec->sample_rate); - goto fail; - } + SANITIZE_PARAMETER(sample_rate, "sample rate", codec->sample_rate < 0, 0) codec->channels = avio_rl16(pb); + SANITIZE_PARAMETER(channels, "number of channels", codec->channels < 0, 0) codec->frame_size = avio_rl16(pb); + SANITIZE_PARAMETER(frame_size, "frame size", codec->frame_size < 0, 0) break; default: goto fail; } if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) { int size = avio_rb32(pb); - codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); - if (!codec->extradata) - return AVERROR(ENOMEM); - codec->extradata_size = size; - avio_read(pb, codec->extradata, size); + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { + av_log(s, AV_LOG_ERROR, "Invalid extradata size %d\n", size); + return AVERROR_INVALIDDATA; + } else { + codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!codec->extradata) + return AVERROR(ENOMEM); + codec->extradata_size = size; + avio_read(pb, codec->extradata, size); + } } avcodec_parameters_from_context(st->codecpar, codec); -- 2.10.2