Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-12 Thread Andreas Cadhalpun 
On 12.01.2016 03:26, Ronald S. Bultje wrote:
> On Mon, Jan 11, 2016 at 12:06 AM, Mats Peterson <
>> On 01/10/2016 11:56 AM, Andreas Cadhalpun wrote:
>>> --- a/libavformat/qtpalette.c
>>> +++ b/libavformat/qtpalette.c
>>> @@ -48,7 +48,7 @@ int ff_get_qtpalette(int codec_id, AVIOContext *pb,
>>> uint32_t *palette)
>>>
>>>   /* If the depth is 1, 2, 4, or 8 bpp, file is palettized. */
>>>   if ((bit_depth == 1 || bit_depth == 2 || bit_depth == 4 ||
>>> bit_depth == 8)) {
>>> -int color_count, color_start, color_end;
>>> +uint32_t color_count, color_start, color_end;
>>>   uint32_t a, r, g, b;
>>>
>>>   /* Ignore the greyscale bit for 1-bit video and sample
>>>
>>>
>> ping
> 
> 
> Why are we using stdint types for non-vector data here? Our custom has
> always been to used sized (stdint-style) data only for vector data (arrays
> etc.), and use native-sized types (e.g. unsigned, int, whatever) for scalar
> values. Why are we making exceptions here?

I can't find this convention in our coding rules[1].
The main reason why I used uint32_t instead of unsigned here was consistency
with the line below. But as Ganesh explained it makes prefect sense to use
uint32_t at least for color_start.

Best regards,
Andreas


1: https://ffmpeg.org/developer.html#Coding-Rules-1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-12 Thread Mats Peterson 

On 01/12/2016 10:44 PM, Andreas Cadhalpun wrote:

Why are we using stdint types for non-vector data here? Our custom has
always been to used sized (stdint-style) data only for vector data (arrays
etc.), and use native-sized types (e.g. unsigned, int, whatever) for scalar
values. Why are we making exceptions here?


I can't find this convention in our coding rules[1].
The main reason why I used uint32_t instead of unsigned here was consistency
with the line below. But as Ganesh explained it makes prefect sense to use
uint32_t at least for color_start.



Yes it does.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Andreas Cadhalpun 
On 10.01.2016 13:03, Mats Peterson wrote:
> On 01/10/2016 11:56 AM, Andreas Cadhalpun wrote:
>> This fixes segmentation faults due to out of bounds writes, when
>> color_start is interpreted as negative number.
>>
> Yes Andreas, until my normalization patch for matroskadec.c is applied, of 
> course
> it's very easy for these variables to be negative when using an int, because 
> of
> the invalid private data. I stand corrected.

It can probably also happen with matroska files, but I saw it crash with
a mov file.

In any case, I pushed the patch now.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Mats Peterson 

On 01/12/2016 03:32 AM, Mats Peterson wrote:

Valid question. Of course there's no problem using uint32_t, but in the
original code the variables are unsigned int... ask Andreas ;)


Well, I'm to blame as well, since I have been using uint32_t for the a, 
r, g and b variables rather than unsigned int, since I thought they 
matched the uint32_t palette[] array better. Is that sensible enough?


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Ganesh Ajjanagadde 
On Mon, Jan 11, 2016 at 9:35 PM, Mats Peterson
 wrote:
> On 01/12/2016 03:32 AM, Mats Peterson wrote:
>>
>> On 01/12/2016 03:26 AM, Ronald S. Bultje wrote:
>>>
>>> Why are we using stdint types for non-vector data here? Our custom has
>>> always been to used sized (stdint-style) data only for vector data
>>> (arrays
>>> etc.), and use native-sized types (e.g. unsigned, int, whatever) for
>>> scalar
>>> values. Why are we making exceptions here?

That is not true; for instance while parsing headers, see avio_rl64.
In fact, really avio_rb32 and the like should return a uint32_t
instead of an unsigned int IMHO. There are of course a variety of
opinions on the subject of stdint types vs native-sized types; and I
doubt there is universal consensus on how liberally to use the stdint
sized types among FFmpeg developers.

>>>
>>> Ronald
>>
>>
>> Valid question. Of course there's no problem using uint32_t, but in the
>> original code the variables are unsigned int... ask Andreas ;)
>>
>> Mats
[...]
> You're free to make another patch, or if perhaps I should do it.

If something is inherently 32 bits (e.g obtained by reading 4 bytes),
then please don't make such a patch.
Seems to be the case here, and so I would nack such a patch:
color_start is obtained by an avio_rb32, keeping as uint32_t is
cleaner.

>
> Mats
>
> --
> Mats Peterson
> http://matsp888.no-ip.org/~mats/
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Mats Peterson 

On 01/12/2016 06:28 AM, Mats Peterson wrote:

Exactly, I actually thought of that myself. And I like the stdint
variables because they eliminate guesswork. That has always been a
problem with the "standard" types in C.


The stdint TYPES, of course.

Mats
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Mats Peterson 

On 01/12/2016 05:48 AM, Ganesh Ajjanagadde wrote:

You're free to make another patch, or if perhaps I should do it.


If something is inherently 32 bits (e.g obtained by reading 4 bytes),
then please don't make such a patch.
Seems to be the case here, and so I would nack such a patch:
color_start is obtained by an avio_rb32, keeping as uint32_t is
cleaner.

No we'll skip it in my book. I don't see reason enough to change it. 
Thanks for your input, Ganesh.


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Mats Peterson 

On 01/12/2016 03:26 AM, Ronald S. Bultje wrote:

Why are we using stdint types for non-vector data here? Our custom has
always been to used sized (stdint-style) data only for vector data (arrays
etc.), and use native-sized types (e.g. unsigned, int, whatever) for scalar
values. Why are we making exceptions here?

Ronald


Valid question. Of course there's no problem using uint32_t, but in the 
original code the variables are unsigned int... ask Andreas ;)


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Ganesh Ajjanagadde 
On Mon, Jan 11, 2016 at 9:48 PM, Mats Peterson
 wrote:
> On 01/12/2016 03:32 AM, Mats Peterson wrote:
>>
>> Valid question. Of course there's no problem using uint32_t, but in the
>> original code the variables are unsigned int... ask Andreas ;)
>
>
> Well, I'm to blame as well, since I have been using uint32_t for the a, r, g
> and b variables rather than unsigned int, since I thought they matched the
> uint32_t palette[] array better. Is that sensible enough?

Don't blame yourself; it is in fact a regression IMHO to change to
unsigned int, albeit a theoretical one. C only guarantees 16 bits for
int/unsigned int, you shift by 24 making it undefined behavior on 16
bit platforms. This is theoretical since POSIX guarantees 32 bits
here; FFmpeg does not support such 16 bit (likely embedded) platforms
anyway. But why change to something worse for no gain ;)?

>
>
> Mats
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Mats Peterson 

On 01/12/2016 05:51 AM, Ganesh Ajjanagadde wrote:

On Mon, Jan 11, 2016 at 9:48 PM, Mats Peterson
 wrote:

On 01/12/2016 03:32 AM, Mats Peterson wrote:

Don't blame yourself; it is in fact a regression IMHO to change to
unsigned int, albeit a theoretical one. C only guarantees 16 bits for
int/unsigned int, you shift by 24 making it undefined behavior on 16
bit platforms. This is theoretical since POSIX guarantees 32 bits
here; FFmpeg does not support such 16 bit (likely embedded) platforms
anyway. But why change to something worse for no gain ;)?




Exactly, I actually thought of that myself. And I like the stdint 
variables because they eliminate guesswork. That has always been a 
problem with the "standard" types in C.


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-11 Thread Mats Peterson 

On 01/12/2016 03:32 AM, Mats Peterson wrote:

On 01/12/2016 03:26 AM, Ronald S. Bultje wrote:

Why are we using stdint types for non-vector data here? Our custom has
always been to used sized (stdint-style) data only for vector data
(arrays
etc.), and use native-sized types (e.g. unsigned, int, whatever) for
scalar
values. Why are we making exceptions here?

Ronald


Valid question. Of course there's no problem using uint32_t, but in the
original code the variables are unsigned int... ask Andreas ;)

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


You're free to make another patch, or if perhaps I should do it.

Mats

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 11:56 AM, Andreas Cadhalpun wrote:

This fixes segmentation faults due to out of bounds writes, when
color_start is interpreted as negative number.

This regression was introduced in commit 57631f.

Signed-off-by: Andreas Cadhalpun 
---

Seriously, changing the code behavior when "factoring out" is a
very bad practice.

---
  libavformat/qtpalette.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/qtpalette.c b/libavformat/qtpalette.c
index a78b6af..666c6b7 100644
--- a/libavformat/qtpalette.c
+++ b/libavformat/qtpalette.c
@@ -48,7 +48,7 @@ int ff_get_qtpalette(int codec_id, AVIOContext *pb, uint32_t 
*palette)

  /* If the depth is 1, 2, 4, or 8 bpp, file is palettized. */
  if ((bit_depth == 1 || bit_depth == 2 || bit_depth == 4 || bit_depth == 
8)) {
-int color_count, color_start, color_end;
+uint32_t color_count, color_start, color_end;
  uint32_t a, r, g, b;

  /* Ignore the greyscale bit for 1-bit video and sample



ping

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Andreas Cadhalpun 
This fixes segmentation faults due to out of bounds writes, when
color_start is interpreted as negative number.

This regression was introduced in commit 57631f.

Signed-off-by: Andreas Cadhalpun 
---

Seriously, changing the code behavior when "factoring out" is a
very bad practice.

---
 libavformat/qtpalette.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/qtpalette.c b/libavformat/qtpalette.c
index a78b6af..666c6b7 100644
--- a/libavformat/qtpalette.c
+++ b/libavformat/qtpalette.c
@@ -48,7 +48,7 @@ int ff_get_qtpalette(int codec_id, AVIOContext *pb, uint32_t 
*palette)
 
 /* If the depth is 1, 2, 4, or 8 bpp, file is palettized. */
 if ((bit_depth == 1 || bit_depth == 2 || bit_depth == 4 || bit_depth == 
8)) {
-int color_count, color_start, color_end;
+uint32_t color_count, color_start, color_end;
 uint32_t a, r, g, b;
 
 /* Ignore the greyscale bit for 1-bit video and sample
-- 
2.6.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 11:56 AM, Andreas Cadhalpun wrote:

This fixes segmentation faults due to out of bounds writes, when
color_start is interpreted as negative number.

dYes Anreas, until my normalization patch for matroskadec.c is applied, 
of course it's very easy for these variables to be negative when using 
an int, because of the invalid private data. I stand corrected.


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 11:56 AM, Andreas Cadhalpun wrote:

This fixes segmentation faults due to out of bounds writes, when
color_start is interpreted as negative number.

This regression was introduced in commit 57631f.

Signed-off-by: Andreas Cadhalpun 
---

Seriously, changing the code behavior when "factoring out" is a
very bad practice.

---
  libavformat/qtpalette.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/qtpalette.c b/libavformat/qtpalette.c
index a78b6af..666c6b7 100644
--- a/libavformat/qtpalette.c
+++ b/libavformat/qtpalette.c
@@ -48,7 +48,7 @@ int ff_get_qtpalette(int codec_id, AVIOContext *pb, uint32_t 
*palette)

  /* If the depth is 1, 2, 4, or 8 bpp, file is palettized. */
  if ((bit_depth == 1 || bit_depth == 2 || bit_depth == 4 || bit_depth == 
8)) {
-int color_count, color_start, color_end;
+uint32_t color_count, color_start, color_end;
  uint32_t a, r, g, b;

  /* Ignore the greyscale bit for 1-bit video and sample



As far as I remember, those variables were ints in the original code in 
mov.c, and they will hardly reach a value where they could be 
interpreted as ints. But it's of course better to be on the safe side. 
Thanks for the heads-up.


Mats
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 12:29 PM, Mats Peterson wrote:

Good advice to a hysterical person like me. Thanks.
At least I'm able to admit that I've been wrong, unlike some other 
people (no names mentioned).


Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Andreas Cadhalpun 
On 10.01.2016 12:13, Mats Peterson wrote:
> On 01/10/2016 11:56 AM, Andreas Cadhalpun wrote:
>> This fixes segmentation faults due to out of bounds writes, when
>> color_start is interpreted as negative number.
>>
>> This regression was introduced in commit 57631f.
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>
>> Seriously, changing the code behavior when "factoring out" is a
>> very bad practice.
>>
>> ---
>>   libavformat/qtpalette.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/qtpalette.c b/libavformat/qtpalette.c
>> index a78b6af..666c6b7 100644
>> --- a/libavformat/qtpalette.c
>> +++ b/libavformat/qtpalette.c
>> @@ -48,7 +48,7 @@ int ff_get_qtpalette(int codec_id, AVIOContext *pb, 
>> uint32_t *palette)
>>
>>   /* If the depth is 1, 2, 4, or 8 bpp, file is palettized. */
>>   if ((bit_depth == 1 || bit_depth == 2 || bit_depth == 4 || bit_depth 
>> == 8)) {
>> -int color_count, color_start, color_end;
>> +uint32_t color_count, color_start, color_end;
>>   uint32_t a, r, g, b;
>>
>>   /* Ignore the greyscale bit for 1-bit video and sample
>>
> 
> As far as I remember, those variables were ints in the original code in mov.c,

You remember wrongly:
$ git show 57631f
[...]
-unsigned int color_start, color_count, color_end;
-unsigned int a, r, g, b;
[...]
+int color_count, color_start, color_end;
+uint32_t a, r, g, b;

> and they will hardly reach a value where they could be interpreted as ints.

Wrong again, as they can be arbitrary values read from the input file:
color_start = avio_rb32(pb);

> But it's of course better to be on the safe side.

Indeed, as otherwise it is a security vulnerability.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Andreas Cadhalpun 
On 10.01.2016 12:24, Mats Peterson wrote:
> On 01/10/2016 12:23 PM, Andreas Cadhalpun wrote:
> 
>> Please check the facts before writing a reply. Thanks.
> 
> It's human to err;) Thanks, anyway.

But it's bad style to send two mails contradicting each other
within 5 minutes.

Just take the 5 minutes to check the facts before sending
the first reply.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 12:29 PM, Andreas Cadhalpun wrote:

Just take the 5 minutes to check the facts before sending
the first reply.

Best regards,
Andreas


Good advice to a hysterical person like me. Thanks.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 01:03 PM, Mats Peterson wrote:

dYes Anreas, until my normalization patch for matroskadec.c is applied,
of course it's very easy for these variables to be negative when using
an int, because of the invalid private data. I stand corrected.

Sorry, ANDREAS. This Thunderbird is killing me.

Mats
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 12:13 PM, Mats Peterson wrote:

As far as I remember, those variables were ints in the original code in
mov.c, and they will hardly reach a value where they could be
interpreted as ints. But it's of course better to be on the safe side.
Thanks for the heads-up.

Interpreted as negative numbers, I mean.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Andreas Cadhalpun 
On 10.01.2016 12:17, Mats Peterson wrote:
> On 01/10/2016 12:14 PM, Mats Peterson wrote:
>> On 01/10/2016 12:13 PM, Mats Peterson wrote:
>>> As far as I remember, those variables were ints in the original code in
>>> mov.c, and they will hardly reach a value where they could be
>>> interpreted as ints. But it's of course better to be on the safe side.
>>> Thanks for the heads-up.
>> Interpreted as negative numbers, I mean.
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> Ah, you're right. They were unsigned ints in the original. My bad.

Please check the facts before writing a reply. Thanks.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 12:23 PM, Andreas Cadhalpun wrote:


Please check the facts before writing a reply. Thanks.


It's human to err;) Thanks, anyway.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] qtpalette: make the color_* variables unsigned again

2016-01-10 Thread Mats Peterson 

On 01/10/2016 12:14 PM, Mats Peterson wrote:

On 01/10/2016 12:13 PM, Mats Peterson wrote:

As far as I remember, those variables were ints in the original code in
mov.c, and they will hardly reach a value where they could be
interpreted as ints. But it's of course better to be on the safe side.
Thanks for the heads-up.

Interpreted as negative numbers, I mean.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Ah, you're right. They were unsigned ints in the original. My bad.

Mats

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel