[FFmpeg-devel] oggparsevorbis.c vorbis_comment: check for negative size

[Michael Niedermayer]

Hi

On Sun, Oct 07, 2007 at 11:33:20AM -0400, Rich Felker wrote:
> On Sun, Oct 07, 2007 at 02:38:10PM +0200, matthieu castet wrote:
> > Attila Kinali wrote:
> > > On Sun, 7 Oct 2007 12:42:13 +0200
> > > Attila Kinali <attila at kinali.ch> wrote:
> > > 
> > > 
> > >> The segfault occures, because s is read from the file but only
> > >> checked to be smaller than the limit, but not whether it is
> > >> positive, resulting in an overflow when it is a big negative number.
> > >>
> > >> Patch attached
> > > 
> > > Updated patch. Missed another occurence of the same problem.
> > Why doesn't you make s unsigned ?
> 
> It won't solve the overflow issue. However checking to make sure s is
> not negative is just a hack to work around the problem of not writing
> overflow-safe unsigned arithmetic.

yes i agree

_maybe_ the following would fix it (ive not checked too carefull)

@@ -somewhere +somewhere @@
    char *p = buf;
-   int s, n, j;
+   int n, j;
+   unsigned int s;


-   if (size < 4)
+   if (size < 8)
        return -1;

    s = AV_RL32(p);
    p += 4;
    size -= 4;

-   if (size < s + 4)
+   if (size - 4 < s)
        return -1;

    p += s;
    size -= s;


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071008/e61ec3c8/attachment.pgp>