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

[Christ-Jan Wijtmans]

why not use size_t


Live long and prosper,

Christ-Jan Wijtmans
https://facebook.com/cj.wijtmans
https://twitter.com/cjwijtmans


On Mon, Feb 25, 2013 at 12:20 AM, Don Moir <donmoir at comcast.net> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel