[FFmpeg-devel] [PATCH 2/3] pmpdec: fix signedness

[Don Moir]

On Sun, Feb 24, 2013 at 11:20:31PM +0100, Michael Niedermayer wrote:
> On Sun, Feb 24, 2013 at 05:06:24PM -0500, Don Moir wrote:
> > ----- Original Message ----- From: "Reimar Döffinger"
> > <Reimar.Doeffinger at gmx.de>
> > To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
> > Sent: Saturday, February 23, 2013 5:46 PM
> > Subject: Re: [FFmpeg-devel] [PATCH 2/3] pmpdec: fix signedness
> >
> > >On 23 Feb 2013, at 22:08, Michael Niedermayer <michaelni at gmx.at> wrote:
> > >
> > >
> > >>>Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > >>>---
> > >>>libavformat/pmpdec.c |    4 ++--
> > >>>1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>>diff --git a/libavformat/pmpdec.c b/libavformat/pmpdec.c
> > >>>index 358f7b6..38eba14 100644
> > >>>--- a/libavformat/pmpdec.c
> > >>>+++ b/libavformat/pmpdec.c
> > >>>@@ -44,7 +44,7 @@ static int pmp_header(AVFormatContext *s)
> > >>>    PMPContext *pmp = s->priv_data;
> > >>>    AVIOContext *pb = s->pb;
> > >>>    int tb_num, tb_den;
> > >>>-    int index_cnt;
> > >>>+    unsigned index_cnt;
> > >>>    int audio_codec_id = AV_CODEC_ID_NONE;
> > >>>    int srate, channels;
> > >>>    int i;
> > >>>@@ -93,7 +93,7 @@ static int pmp_header(AVFormatContext *s)
> > >>>    channels = avio_rl32(pb) + 1;
> > >>>    pos = avio_tell(pb) + 4*index_cnt;
> > >>>    for (i = 0; i < index_cnt; i++) {
> > >>>-        int size = avio_rl32(pb);
> > >>>+        unsigned size = avio_rl32(pb);
> >
> > >>Why not go all the way and use uint32_t ?
> > >>Seems safest to do.
> > >>All patches look ok to me.
> >
> > >By changing size and index_cnt to uint32_t it introduces 3
> > >warnings for signed/unsigned mismatch for me when compiling that
> > >code in Visual Studio. You don't see that in mingw. But more than
> > >that, it seems to me by keeping them as int you get an automatic
> > >check for an out of bounds condition. That is, an exceesively
> > >large number will be negative and fail accordingly.
> >
>
> > Also if index_cnt is a number greater than INT_MAX since it's now uint32_t and since ' i ' is an int, an endless loop will 
> > occur.
>
> no

>a comparission between signed and unsigend int is of unsigned type.
>posix gurantees int has at least 32bit

I was unaware since I don't mismatch signs or I make it explicit when needed.

>also theres a EOF check in the loop

Yes and it fixed the overall problem.

>...but probably better to fix it anyway

Yes. Not sure why the signs were changed to begin with. Is a greater than INT_MAX value expected here ? Seems that would be a sure 
sign of error.