[FFmpeg-devel] [PATCH] oggdec: add integer overflow and allocation check in ogg_read_page()
Michael Niedermayer
michaelni at gmx.at
Tue May 24 14:34:32 CEST 2011
On Tue, May 24, 2011 at 01:48:46PM +0200, Stefano Sabatini wrote:
> On date Tuesday 2011-05-24 04:04:00 +0200, Michael Niedermayer encoded:
> > On Tue, May 24, 2011 at 12:36:23AM +0200, Stefano Sabatini wrote:
> > > On date Monday 2011-05-23 19:15:45 +0200, Michael Niedermayer encoded:
> > > > On Mon, May 23, 2011 at 06:44:11PM +0200, Stefano Sabatini wrote:
> > > > > On date Monday 2011-05-23 05:15:27 +0200, Michael Niedermayer encoded:
> > > > > > On Mon, May 23, 2011 at 12:04:29AM +0200, Stefano Sabatini wrote:
> > > > > > > ---
> > > > > > > libavformat/oggdec.c | 8 +++++++-
> > > > > > > 1 files changed, 7 insertions(+), 1 deletions(-)
> > > > > > >
> > > > > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > > > > index 7f65365..f137b97 100644
> > > > > > > --- a/libavformat/oggdec.c
> > > > > > > +++ b/libavformat/oggdec.c
> > > > > > > @@ -288,7 +288,13 @@ static int ogg_read_page(AVFormatContext *s, int *str)
> > > > > > > }
> > > > > > >
> > > > > > > if (os->bufsize - os->bufpos < size){
> > > > > > > - uint8_t *nb = av_malloc (os->bufsize *= 2);
> > > > > > > + uint8_t *nb;
> > > > > > > + if (os->bufsize > SIZE_MAX/2) {
> > > > > > > + av_log(s, AV_LOG_ERROR, "Ogg page with size %u is too big\n", os->bufsize);
> > > > > > > + return AVERROR_INVALIDDATA;
> > > > > > > + }
> > > > > > > + if (!(nb = av_malloc(os->bufsize *= 2)))
> > > > > > > + return AVERROR(ENOMEM);
> > > > > >
> > > > > > i hope there is a better solution than allocating several gigabyte
> > > > >
> > > > > Yes, but this at least is fixing a crash.
> > > >
> > > > please review attached patch
> > > > note this is a RFC, i have not checked if this has sideeffects and
> > > > i do not know why the if() was there.
> > >
> > > commit 40c5e1fa2e5fd668ed69528d91521b46ec64f96a
> > > Author: Måns Rullgård <mans at mansr.com>
> > > Date: Sun Jun 25 12:23:54 2006 +0000
> > >
> > > 10l: don't allocate a new buffer quite so often
> > >
> > > Originally committed as revision 5523 to svn://svn.ffmpeg.org/ffmpeg/trunk
> > >
> > > diff --git a/libavformat/ogg2.c b/libavformat/ogg2.c
> > > index 9cb3d8c..b29bfe9 100644
> > > --- a/libavformat/ogg2.c
> > > +++ b/libavformat/ogg2.c
> > > @@ -193,6 +193,7 @@ ogg_new_stream (AVFormatContext * s, uint32_t serial)
> > > os = ogg->streams + idx;
> > > os->serial = serial;
> > > os->bufsize = DECODER_BUFFER_SIZE;
> > > + os->buf = av_malloc(os->bufsize);
> > > os->header = -1;
> > >
> > > st = av_new_stream (s, idx);
> > > @@ -279,7 +280,7 @@ ogg_read_page (AVFormatContext * s, int *str)
> > >
> > > os = ogg->streams + idx;
> > >
> > > - if(os->segp == os->nsegs)
> > > + if(os->psize > 0)
> > > ogg_new_buf(ogg, idx);
> > >
> > > > --
> > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > > >
> > > > Asymptotically faster algorithms should always be preferred if you have
> > > > asymptotical amounts of data
> > >
> > > > From ea6e633d62b7ab7deac746e49e41f1754ae18a0f Mon Sep 17 00:00:00 2001
> > > > From: Michael Niedermayer <michaelni at gmx.at>
> > > > Date: Mon, 23 May 2011 19:10:15 +0200
> > > > Subject: [PATCH] ioggdec: fix runaway allocation
> > > >
> > > > fixes ticket185
> > > >
> > > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > > ---
> > > > libavformat/oggdec.c | 3 +--
> > > > 1 files changed, 1 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > index 7f65365..c90e222 100644
> > > > --- a/libavformat/oggdec.c
> > > > +++ b/libavformat/oggdec.c
> > > > @@ -258,8 +258,7 @@ static int ogg_read_page(AVFormatContext *s, int *str)
> > > > os = ogg->streams + idx;
> > > > os->page_pos = avio_tell(bc) - 27;
> > > >
> > > > - if(os->psize > 0)
> > > > - ogg_new_buf(ogg, idx);
> > > > + ogg_new_buf(ogg, idx);
> > >
> > > From what I see ogg_new_buf() moves the data at the begin of the
> > > buffer, creating a new buffer when the data is memcpyied (BTW
> > > ogg_new_buf() is not a particularly helpful name).
> > >
> > > I suppose a better solution would be to use a fifo. Maybe the check
> > > was added for avoiding continuos reallocation of the buffer, so
> > > removing the check may have a performance penalty, but right at the
> >
> > memmove() could be used avoiding realloc.
> > and when size is 0 not even that is needed
>
> Anyway, back to the original problem, please see attached
> patchset.
>
> Your patch can be applied on top of that, the issue reporter tells
> that the patch fixes the issue, then we noted there are other memory
> issues but I want to leave them for now, and possibly wait for a reply
> from David Conrad (listed as file maintainer).
> --
> FFmpeg = Faithful and Fostering Murdering Perennial Elfic Glue
> oggdec.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 2ffdc2480b9754eb0981b0072556ce094bd07c72 0001-oggdec-add-integer-overflow-and-allocation-check-in-.patch
> From 3e7461df29cca18749db557b1e6616f578b3b73a Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Thu, 19 May 2011 00:05:21 +0200
> Subject: [PATCH] oggdec: add integer overflow and allocation check in ogg_read_page()
>
> Should fix trac issue #185.
> ---
> libavformat/oggdec.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 344bd1c..aa63f96 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -288,7 +288,13 @@ static int ogg_read_page(AVFormatContext *s, int *str)
> }
>
> if (os->bufsize - os->bufpos < size){
> - uint8_t *nb = av_malloc (os->bufsize *= 2);
> + uint8_t *nb;
> + if (os->bufsize > SIZE_MAX/2) {
bufsize is unsigned int SIZE_MAX can be larger
> + av_log(s, AV_LOG_ERROR, "Ogg page with size %u is too big\n", os->bufsize);
> + return AVERROR_INVALIDDATA;
> + }
> + if (!(nb = av_malloc(os->bufsize *= 2)))
> + return AVERROR(ENOMEM);
> memcpy (nb, os->buf, os->bufpos);
> av_free (os->buf);
> os->buf = nb;
> --
> 1.7.2.3
>
> oggdec.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
> 7f6d95628a80cf80837ddf9364b94f8dfeb5098b 0002-oggdec-add-various-malloc-and-NULL-checks.patch
> From cd92ec295cddc077bf7ae0e2838ac8ddac664d44 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Tue, 24 May 2011 13:11:59 +0200
> Subject: [PATCH] oggdec: add various malloc and NULL checks
>
> ---
> libavformat/oggdec.c | 22 +++++++++++++++++++---
> 1 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index aa63f96..07d2bee 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -59,9 +59,12 @@ static const struct ogg_codec * const ogg_codecs[] = {
> static int ogg_save(AVFormatContext *s)
> {
> struct ogg *ogg = s->priv_data;
> + int i;
> struct ogg_state *ost =
> av_malloc(sizeof (*ost) + (ogg->nstreams-1) * sizeof (*ogg->streams));
> - int i;
> + if (!ost)
> + return AVERROR(ENOMEM);
the return value of ogg_save isnt checked
> +
> ost->pos = avio_tell (s->pb);
> ost->curidx = ogg->curidx;
> ost->next = ogg->state;
> @@ -71,6 +74,11 @@ static int ogg_save(AVFormatContext *s)
> for (i = 0; i < ogg->nstreams; i++){
> struct ogg_stream *os = ogg->streams + i;
> os->buf = av_malloc (os->bufsize);
> + if (!os->buf) {
> + while (i--)
> + av_freep(&ogg->streams[i]);
> + return AVERROR(ENOMEM);
> + }
> memset (os->buf, 0, os->bufsize);
> memcpy (os->buf, ost->streams[i].buf, os->bufpos);
> }
> @@ -154,11 +162,15 @@ static int ogg_new_stream(AVFormatContext *s, uint32_t serial, int new_avstream)
>
> ogg->streams = av_realloc (ogg->streams,
> ogg->nstreams * sizeof (*ogg->streams));
> + if (!ogg->streams)
> + return AVERROR(ENOMEM);
this will leak
> memset (ogg->streams + idx, 0, sizeof (*ogg->streams));
> os = ogg->streams + idx;
> os->serial = serial;
> os->bufsize = DECODER_BUFFER_SIZE;
> os->buf = av_malloc(os->bufsize);
> + if (!os->buf)
> + return AVERROR(ENOMEM);
> os->header = -1;
>
> if (new_avstream) {
> @@ -177,6 +189,8 @@ static int ogg_new_buf(struct ogg *ogg, int idx)
> struct ogg_stream *os = ogg->streams + idx;
> uint8_t *nb = av_malloc(os->bufsize);
> int size = os->bufpos - os->pstart;
> + if (!nb)
> + return AVERROR(ENOMEM);
> if(os->buf){
> memcpy(nb, os->buf + os->pstart, size);
> av_free(os->buf);
> @@ -601,8 +615,10 @@ static int ogg_read_close(AVFormatContext *s)
> int i;
>
> for (i = 0; i < ogg->nstreams; i++){
> - av_free (ogg->streams[i].buf);
> - av_free (ogg->streams[i].private);
> + if (ogg->streams[i]) {
> + av_free(ogg->streams[i].buf);
> + av_free(ogg->streams[i].private);
> + }
> }
> av_free (ogg->streams);
> return 0;
> --
> 1.7.2.3
>
> oggdec.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 5fe0ecad0d01e979206082c50c74ee8c3c6bc51e 0003-oggdec-in-ogg_save-use-av_mallocz-for-resetting-allo.patch
> From acb6d0ee7b40d0a4dadf220f4ff394d10c782551 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Tue, 24 May 2011 13:12:59 +0200
> Subject: [PATCH] oggdec: in ogg_save(), use av_mallocz() for resetting allocated memory
>
> Simplify.
ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110524/03f870eb/attachment.asc>
More information about the ffmpeg-devel
mailing list