[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.

>a count (of index entries) cant be negative thus it has to be
>unsigned 32bit or >32bit
>we surely could disallow values that large but the code would not
>be simpler.
>there are multiple more or less equivalent ways to solve things

Yeah I get you. Let it go eof and error out that way. While not quite as clear in that the reported error may be misleading, you are 
right in that it produces the simplest code.