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

[Michael Niedermayer]

On Sun, Feb 24, 2013 at 06:20:41PM -0500, Don Moir wrote:
> 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

you can check for a arbitrary choosen maximum or support any storeable
value, ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- 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/20130225/48c7b5f0/attachment.asc>