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

[Michael Niedermayer]

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
also theres a EOF check in the loop

i guess in a few minutes some language lawyer will tell us that the
i++ is undefined behavior :)
(which has no practical relevance here but probably better to fix it
 anyway)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20130224/45d02f23/attachment.asc>